diff --git a/src-tauri/crates/git/src/watch/event_emitter.rs b/src-tauri/crates/git/src/watch/event_emitter.rs index 20d7d83768..2d9a0adfe4 100644 --- a/src-tauri/crates/git/src/watch/event_emitter.rs +++ b/src-tauri/crates/git/src/watch/event_emitter.rs @@ -43,13 +43,15 @@ impl EventEmitter { }); let payload = json!({ + "type": "repo:changed", "repo_id": repo_id, "change_type": change_type_str, "affected_count": affected_count, "timestamp": Self::current_timestamp_ms(), }); - let _ = self.app_handle.emit("repo:changed", payload); + let _ = self.app_handle.emit("repo:changed", payload.clone()); + crate::hooks::websocket_broadcast(payload.to_string()); } /// Emit file changed event (for Filesync channel - individual file changes) diff --git a/src-tauri/crates/git/src/watch/state_store.rs b/src-tauri/crates/git/src/watch/state_store.rs index f3394f4962..9de0691101 100644 --- a/src-tauri/crates/git/src/watch/state_store.rs +++ b/src-tauri/crates/git/src/watch/state_store.rs @@ -86,6 +86,25 @@ impl RepoStateStore { .collect() } + /// Snapshot only the fields needed to enforce the watch-set capacity. + /// + /// Like `get_unhealthy_watcher_ids`, this deliberately avoids cloning the + /// cached `GitStatus` each state may own. + pub fn get_watch_activity(&self) -> Vec { + self.states + .read() + .values() + .map(|state| WatchActivity { + repo_id: state.repo_id.clone(), + last_activity: state + .last_git_status_ts + .unwrap_or(state.last_fs_event_ts) + .max(state.last_fs_event_ts), + has_in_flight_jobs: !state.in_flight_jobs.is_empty(), + }) + .collect() + } + // ============================================ // Dirty Flag Management // ============================================ diff --git a/src-tauri/crates/git/src/watch/tests/watcher_tests.rs b/src-tauri/crates/git/src/watch/tests/watcher_tests.rs index 69cd587a6d..f387a024f6 100644 --- a/src-tauri/crates/git/src/watch/tests/watcher_tests.rs +++ b/src-tauri/crates/git/src/watch/tests/watcher_tests.rs @@ -1,8 +1,9 @@ use notify::{event::ModifyKind, Event, EventKind}; use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; -use crate::watch::types::RepoChangeType; -use crate::watch::watcher::RepoWatcher; +use crate::watch::types::{RepoChangeType, WatchActivity}; +use crate::watch::watcher::{select_watch_eviction_victims, RepoWatcher}; fn make_event(paths: Vec) -> Event { Event { @@ -12,6 +13,53 @@ fn make_event(paths: Vec) -> Event { } } +fn watch_activity(repo_id: &str, age: Duration, has_in_flight_jobs: bool) -> WatchActivity { + WatchActivity { + repo_id: repo_id.to_string(), + last_activity: Instant::now() - age, + has_in_flight_jobs, + } +} + +// ============================================ +// watch capacity eviction +// ============================================ + +#[test] +fn watch_capacity_evicts_oldest_idle_repositories() { + let activity = vec![ + watch_activity("recent", Duration::from_secs(10), false), + watch_activity("oldest", Duration::from_secs(30), false), + watch_activity("older", Duration::from_secs(20), false), + ]; + + assert_eq!( + select_watch_eviction_victims(&activity, &[], 2), + vec!["oldest".to_string(), "older".to_string()] + ); +} + +#[test] +fn watch_capacity_preserves_protected_and_busy_repositories() { + let activity = vec![ + watch_activity("active-poll", Duration::from_secs(40), false), + watch_activity("busy", Duration::from_secs(30), true), + watch_activity("eligible", Duration::from_secs(20), false), + ]; + + assert_eq!( + select_watch_eviction_victims(&activity, &["active-poll", "incoming"], 3), + vec!["eligible".to_string()] + ); +} + +#[test] +fn watch_capacity_does_nothing_without_overflow() { + let activity = vec![watch_activity("repo", Duration::from_secs(10), false)]; + + assert!(select_watch_eviction_victims(&activity, &[], 0).is_empty()); +} + // ============================================ // classify_event: Windows backslash paths // ============================================ diff --git a/src-tauri/crates/git/src/watch/types.rs b/src-tauri/crates/git/src/watch/types.rs index 5564ac65cc..ee374ee2a5 100644 --- a/src-tauri/crates/git/src/watch/types.rs +++ b/src-tauri/crates/git/src/watch/types.rs @@ -85,6 +85,19 @@ impl RepoState { } } +/// Lightweight per-repo record used to pick watch-capacity eviction victims. +/// +/// Cloning whole `RepoState` values would allocate in proportion to every +/// cached status file list; capacity checks only need identity, recency, and +/// whether work is still in flight. +#[derive(Debug, Clone)] +pub struct WatchActivity { + pub repo_id: String, + /// Most recent of the last filesystem event and the last status refresh. + pub last_activity: Instant, + pub has_in_flight_jobs: bool, +} + // ============================================ // Git Status // ============================================ diff --git a/src-tauri/crates/git/src/watch/watcher.rs b/src-tauri/crates/git/src/watch/watcher.rs index 0fd4485ef8..877af91e14 100644 --- a/src-tauri/crates/git/src/watch/watcher.rs +++ b/src-tauri/crates/git/src/watch/watcher.rs @@ -128,6 +128,44 @@ const DEBOUNCED_GIT_PATHS: &[&str] = &[ ".git/index", // Staging area - changes on every git add/rm ]; +// ============================================ +// Watch Capacity +// ============================================ + +/// Pick the least-recently-active watched repos to evict so the watch set +/// honors `MAX_WATCHED_REPOS`. +/// +/// Repos listed in `protected` (the incoming repo and the repo currently +/// driving working-directory polling) and repos with in-flight jobs are never +/// chosen — dropping their watcher would strand work the user can still see. +/// When everything is protected the set is allowed to exceed the cap rather +/// than break a live surface. +pub(crate) fn select_watch_eviction_victims( + activity: &[WatchActivity], + protected: &[&str], + overflow: usize, +) -> Vec { + if overflow == 0 { + return Vec::new(); + } + + let mut candidates: Vec<&WatchActivity> = activity + .iter() + .filter(|entry| !entry.has_in_flight_jobs && !protected.contains(&entry.repo_id.as_str())) + .collect(); + candidates.sort_by(|left, right| { + left.last_activity + .cmp(&right.last_activity) + .then_with(|| left.repo_id.cmp(&right.repo_id)) + }); + + candidates + .into_iter() + .take(overflow) + .map(|entry| entry.repo_id.clone()) + .collect() +} + // ============================================ // RepoWatcher // ============================================ @@ -445,6 +483,8 @@ impl RepoWatcher { return Err(format!("Not a git repository: {:?}", repo_path)); } + self.enforce_watch_capacity(&repo_info.repo_id); + // Add to state store and wake the poller if it was parked with no active repos. self.state_store.add_repo(repo_info.clone()); { @@ -525,6 +565,46 @@ impl RepoWatcher { Ok(()) } + /// Drop the least-recently-active watchers so the watch set stays within + /// `MAX_WATCHED_REPOS`. + /// + /// Every retained repo holds a native watcher (file descriptors) plus a + /// cached `GitStatus` whose file list scales with the working tree, and + /// nothing unwatches a repo the user simply navigated away from — so an + /// uncapped watch set grows for the whole app session as projects are + /// opened. An evicted repo keeps working through the existing + /// polling/on-demand paths and is re-watched on its next `watch_repo`. + fn enforce_watch_capacity(&self, incoming_repo_id: &str) { + let activity = self.state_store.get_watch_activity(); + if activity + .iter() + .any(|entry| entry.repo_id == incoming_repo_id) + { + // Re-watching an already-tracked repo does not grow the set. + return; + } + + let overflow = (activity.len() + 1).saturating_sub(MAX_WATCHED_REPOS); + if overflow == 0 { + return; + } + + let active_poll_repo_id = self.active_poll_repo_id.read().clone(); + let mut protected: Vec<&str> = vec![incoming_repo_id]; + if let Some(active_repo_id) = active_poll_repo_id.as_deref() { + protected.push(active_repo_id); + } + + for repo_id in select_watch_eviction_victims(&activity, &protected, overflow) { + log::info!( + "[RepoWatch] Watch capacity {} reached; evicting least-recently-active repo {}", + MAX_WATCHED_REPOS, + repo_id + ); + let _ = self.unwatch_repo(&repo_id); + } + } + /// Stop watching a repository pub fn unwatch_repo(&self, repo_id: &str) -> Result<(), String> { // Remove watcher diff --git a/src-tauri/crates/search/src/file.rs b/src-tauri/crates/search/src/file.rs index 62279d27d0..a8196fd404 100644 --- a/src-tauri/crates/search/src/file.rs +++ b/src-tauri/crates/search/src/file.rs @@ -15,12 +15,16 @@ use nucleo_matcher::pattern::{CaseMatching, Normalization, Pattern}; use nucleo_matcher::{Config, Matcher, Utf32Str}; use rayon::prelude::*; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; use std::path::PathBuf; -use std::sync::{Arc, Mutex}; -use std::time::Instant; +use std::sync::{Arc, LazyLock}; +use std::time::{Duration, Instant}; use tracing::{debug, info, warn}; +#[path = "file/index_cache.rs"] +mod index_cache; + +use index_cache::FilePathIndexCache; + // ============================================ // Types // ============================================ @@ -62,86 +66,34 @@ struct FileEntry { is_dir: bool, } -struct FileIndex { - entries: Arc>, - _root_path: String, - indexed_at: std::time::SystemTime, - estimated_bytes: usize, -} - -/// Cache TTL — 5 minutes. The old 30 s TTL caused a cold re-walk every time -/// the user paused for half a minute between @ searches. -const CACHE_TTL_SECS: u64 = 300; +/// File changes invalidate indexes through the repository watcher. This slow +/// safety TTL only recovers from a missed watcher event; it is not a polling +/// cadence and does not create background work by itself. +const CACHE_SAFETY_TTL: Duration = Duration::from_secs(60 * 60); const MAX_CACHED_FILE_INDEXES: usize = 4; -const MAX_FILE_INDEX_BYTES: usize = 32 * 1024 * 1024; -const MAX_FILE_INDEX_CACHE_BYTES: usize = 64 * 1024 * 1024; - -static FILE_INDEX_CACHE: std::sync::LazyLock>>> = - std::sync::LazyLock::new(|| Arc::new(Mutex::new(HashMap::new()))); - -fn prune_file_index_cache(cache: &mut HashMap) { - cache.retain(|_, index| { - index - .indexed_at - .elapsed() - .map(|elapsed| elapsed.as_secs() < CACHE_TTL_SECS) - .unwrap_or(false) - }); - let mut total_bytes = cache - .values() - .map(|index| index.estimated_bytes) - .sum::(); - while cache.len() > MAX_CACHED_FILE_INDEXES || total_bytes > MAX_FILE_INDEX_CACHE_BYTES { - let Some(oldest_key) = cache - .iter() - .min_by_key(|(_, index)| index.indexed_at) - .map(|(root_path, _)| root_path.clone()) - else { - break; - }; - if let Some(removed) = cache.remove(&oldest_key) { - total_bytes = total_bytes.saturating_sub(removed.estimated_bytes); - } - } -} - -fn estimate_file_index_bytes(entries: &[FileEntry]) -> usize { - std::mem::size_of_val(entries) - + entries - .iter() - .map(|entry| entry.path.len() + entry.filename.len()) - .sum::() -} - -fn insert_file_index_cache_entry( - root_path: String, - entries: Arc>, - indexed_at: std::time::SystemTime, -) { - let estimated_bytes = estimate_file_index_bytes(&entries); - if estimated_bytes > MAX_FILE_INDEX_BYTES { - debug!( - root_path = %root_path, - entries = entries.len(), - estimated_bytes, - "search::file: index exceeds per-repository cache budget; using it for this request only" - ); - return; - } - - let mut cache = FILE_INDEX_CACHE.lock().unwrap(); - prune_file_index_cache(&mut cache); - cache.insert( - root_path.clone(), - FileIndex { - entries, - _root_path: root_path, - indexed_at, - estimated_bytes, - }, - ); - prune_file_index_cache(&mut cache); +static FILE_INDEX_CACHE: LazyLock = + LazyLock::new(|| FilePathIndexCache::new(CACHE_SAFETY_TTL, MAX_CACHED_FILE_INDEXES)); + +const DEFAULT_EXCLUDED_DIRS: &[&str] = &[ + "node_modules", + ".git", + "dist", + "build", + ".next", + "target", + ".cache", + "coverage", + "__pycache__", + ".venv", + "venv", +]; + +fn default_excluded_dirs() -> Vec { + DEFAULT_EXCLUDED_DIRS + .iter() + .map(|directory| (*directory).to_string()) + .collect() } // ============================================ @@ -173,17 +125,29 @@ fn build_file_index(root_path: &str, exclude_dirs: &[String]) -> Vec // Skip excluded directories at the walker level so we never descend // into node_modules, .git, etc. This is orders of magnitude faster // than post-filtering. + let root = std::path::PathBuf::from(root_path); + let filter_root = root.clone(); builder.filter_entry(move |entry| { if entry.file_type().is_some_and(|ft| ft.is_dir()) { let name = entry.file_name().to_string_lossy(); if exclude_set.contains(name.as_ref()) { return false; } + + // ORG2 runtime worktrees contain full repository copies and their + // generated artifacts. They are implementation state, not distinct + // user files, so descending into them multiplies every index walk. + if let Ok(relative) = entry.path().strip_prefix(&filter_root) { + if relative == std::path::Path::new(".worktrees") + || relative == std::path::Path::new(".orgii/worktrees") + { + return false; + } + } } true }); - let root = std::path::Path::new(root_path); let walker = builder.build(); let entries: Vec = walker @@ -224,42 +188,21 @@ fn build_file_index(root_path: &str, exclude_dirs: &[String]) -> Vec /// **never** during the expensive `build_file_index` walk. This means /// concurrent searches for different repos proceed in parallel, and a /// slow index build for repo A won't block a cached lookup for repo B. -fn get_file_index(root_path: &str, exclude_dirs: &[String]) -> Arc> { - // 1. Quick check under the lock — return cached entries if fresh. - { - let mut cache = FILE_INDEX_CACHE.lock().unwrap(); - prune_file_index_cache(&mut cache); - if let Some(index) = cache.get(root_path) { - if let Ok(elapsed) = index.indexed_at.elapsed() { - if elapsed.as_secs() < CACHE_TTL_SECS { - return Arc::clone(&index.entries); - } - } - } - } // ← lock released here - - // 2. Validate the path before spending time walking it. - // Protects against bad descriptors after rapid repo switches. +fn get_file_index(root_path: &str, exclude_dirs: &[String]) -> Result, String> { + // Validate the path before spending time walking it. Protects against bad + // descriptors after rapid repo switches. let root = std::path::Path::new(root_path); if !root.exists() || !root.is_dir() { warn!( root_path = %root_path, "search::file: root path invalid or gone; skipping index" ); - return Arc::new(Vec::new()); + return Ok(Arc::from(Vec::::new())); } - // 3. Build index WITHOUT holding the lock. - let entries = Arc::new(build_file_index(root_path, exclude_dirs)); - - // 4. Re-acquire lock to store. - insert_file_index_cache_entry( - root_path.to_string(), - Arc::clone(&entries), - std::time::SystemTime::now(), - ); - - entries + FILE_INDEX_CACHE.get_or_build(root_path, exclude_dirs, || { + build_file_index(root_path, exclude_dirs) + }) } // ============================================ @@ -271,29 +214,25 @@ fn score_entry( entry: &FileEntry, pattern: &Pattern, matcher: &mut Matcher, -) -> Option<(FileEntry, i64)> { - // Buffer for UTF-32 conversion - let mut buf = Vec::new(); + buf: &mut Vec, +) -> Option { + buf.clear(); // Convert filename to Utf32Str for nucleo - let filename_utf32 = Utf32Str::new(&entry.filename, &mut buf); + let filename_utf32 = Utf32Str::new(&entry.filename, buf); // Try matching against filename first (higher priority) if let Some(score) = pattern.score(filename_utf32, matcher) { // Boost filename matches significantly let boosted_score = (score as i64) * 2; - return Some((entry.clone(), boosted_score)); + return Some(boosted_score); } // Clear buffer and try matching against full path buf.clear(); - let path_utf32 = Utf32Str::new(&entry.path, &mut buf); - - if let Some(score) = pattern.score(path_utf32, matcher) { - return Some((entry.clone(), score as i64)); - } + let path_utf32 = Utf32Str::new(&entry.path, buf); - None + pattern.score(path_utf32, matcher).map(i64::from) } /// Perform fuzzy search on the file index @@ -325,9 +264,11 @@ fn fuzzy_search( ); // Use parallel processing for large indices - let results: Vec<(FileEntry, i64)> = entries + let mut scored_results: Vec<(usize, i64)> = entries .par_iter() + .enumerate() .filter(|entry| { + let entry = entry.1; // Filter by extension if specified if let Some(extensions) = file_extensions { if !entry.is_dir { @@ -339,19 +280,33 @@ fn fuzzy_search( } true }) - .filter_map(|entry| { - // Each thread gets its own matcher - let mut matcher = Matcher::new(Config::DEFAULT); - score_entry(entry, &pattern, &mut matcher) - }) + .map_init( + || (Matcher::new(Config::DEFAULT), Vec::new()), + |(matcher, buf), (index, entry)| { + score_entry(entry, &pattern, matcher, buf).map(|score| (index, score)) + }, + ) + .filter_map(|result| result) .collect(); - // Sort by score descending and take top results - let mut sorted_results = results; - sorted_results.sort_by_key(|result| std::cmp::Reverse(result.1)); - sorted_results.truncate(max_results); + if max_results == 0 { + return Vec::new(); + } + + // Partition first so only the requested top-K needs a full sort. + let compare_rank = |left: &(usize, i64), right: &(usize, i64)| { + right.1.cmp(&left.1).then_with(|| left.0.cmp(&right.0)) + }; + if scored_results.len() > max_results { + scored_results.select_nth_unstable_by(max_results, compare_rank); + scored_results.truncate(max_results); + } + scored_results.sort_unstable_by(compare_rank); - sorted_results + scored_results + .into_iter() + .map(|(index, score)| (entries[index].clone(), score)) + .collect() } // ============================================ @@ -371,30 +326,23 @@ pub async fn search_files_fuzzy(options: SearchOptions) -> Result = Vec::new(); @@ -450,33 +398,16 @@ pub async fn index_project_files( } // Default exclusions - let default_excludes = vec![ - "node_modules".to_string(), - ".git".to_string(), - "dist".to_string(), - "build".to_string(), - ".next".to_string(), - "target".to_string(), - ]; + let default_excludes = default_excluded_dirs(); let exclude_dirs = exclude_dirs.unwrap_or(default_excludes); - // Clear existing cache for this path - { - let mut cache = FILE_INDEX_CACHE.lock().unwrap(); - cache.remove(&root_path); - } - - // Build fresh index - let entries = Arc::new(build_file_index(&root_path, &exclude_dirs)); + // Invalidate every exclusion-policy variant for this root. A build + // that started before this force request cannot repopulate the cache. + FILE_INDEX_CACHE.invalidate_root(&root_path); + let entries = get_file_index(&root_path, &exclude_dirs)?; let count = entries.len(); - insert_file_index_cache_entry( - root_path, - Arc::clone(&entries), - std::time::SystemTime::now(), - ); - let duration = start.elapsed(); info!(entries = count, ?duration, "search::file: indexed entries"); @@ -502,50 +433,12 @@ pub async fn prewarm_file_index(root_path: String) -> Result { )); } - // Check if already cached and fresh — skip the walk entirely. - { - let mut cache = FILE_INDEX_CACHE.lock().unwrap(); - prune_file_index_cache(&mut cache); - if let Some(index) = cache.get(&root_path) { - if let Ok(elapsed) = index.indexed_at.elapsed() { - if elapsed.as_secs() < CACHE_TTL_SECS { - debug!( - entries = index.entries.len(), - age_secs = elapsed.as_secs_f64(), - "search::file: prewarm skipped; cache still fresh" - ); - return Ok(index.entries.len()); - } - } - } - } - debug!(root_path = %root_path, "search::file: prewarming index"); - let default_excludes = vec![ - "node_modules".to_string(), - ".git".to_string(), - "dist".to_string(), - "build".to_string(), - ".next".to_string(), - "target".to_string(), - ".cache".to_string(), - "coverage".to_string(), - "__pycache__".to_string(), - ".venv".to_string(), - "venv".to_string(), - ]; - - // Build WITHOUT holding the lock. - let entries = Arc::new(build_file_index(&root_path, &default_excludes)); + let default_excludes = default_excluded_dirs(); + let entries = get_file_index(&root_path, &default_excludes)?; let count = entries.len(); - insert_file_index_cache_entry( - root_path, - Arc::clone(&entries), - std::time::SystemTime::now(), - ); - info!(entries = count, "search::file: prewarm complete"); Ok(count) }) @@ -556,11 +449,20 @@ pub async fn prewarm_file_index(root_path: String) -> Result { /// Clear the file index cache #[tauri::command] pub fn clear_file_index_cache() { - let mut cache = FILE_INDEX_CACHE.lock().unwrap(); - cache.clear(); + FILE_INDEX_CACHE.clear(); info!("search::file: cache cleared"); } +/// Invalidate cached file indexes for one workspace root. +/// +/// This command performs no scan. The next foreground prewarm or search builds +/// a fresh index, and any older in-flight generation is discarded. +#[tauri::command] +pub fn invalidate_file_index_cache(root_path: String) { + FILE_INDEX_CACHE.invalidate_root(&root_path); + debug!(root_path = %root_path, "search::file: root cache invalidated"); +} + /// Find files by extension in a directory /// Returns list of file paths matching any of the given extensions #[tauri::command] @@ -584,21 +486,8 @@ pub async fn find_files_by_extension( } // Directories to skip entirely (the walker will NOT descend into them). - let exclude_set: std::collections::HashSet = [ - "node_modules", - ".git", - "dist", - "build", - ".next", - "target", - ".cache", - "__pycache__", - ".venv", - "venv", - ] - .iter() - .map(|s| s.to_string()) - .collect(); + let exclude_set: std::collections::HashSet = + default_excluded_dirs().into_iter().collect(); let mut builder = WalkBuilder::new(&directory); diff --git a/src-tauri/crates/search/src/file/index_cache.rs b/src-tauri/crates/search/src/file/index_cache.rs new file mode 100644 index 0000000000..47bfd787cb --- /dev/null +++ b/src-tauri/crates/search/src/file/index_cache.rs @@ -0,0 +1,392 @@ +use super::FileEntry; +use std::collections::HashMap; +use std::panic::{catch_unwind, AssertUnwindSafe}; +use std::sync::{Arc, Condvar, Mutex}; +use std::time::{Duration, Instant}; + +#[derive(Debug, Clone, Hash, PartialEq, Eq)] +struct FileIndexKey { + root_path: String, + exclude_dirs: Vec, +} + +impl FileIndexKey { + fn new(root_path: &str, exclude_dirs: &[String]) -> Self { + let mut exclude_dirs = exclude_dirs.to_vec(); + exclude_dirs.sort_unstable(); + exclude_dirs.dedup(); + Self { + root_path: root_path.to_string(), + exclude_dirs, + } + } +} + +struct CachedIndex { + entries: Arc<[FileEntry]>, + indexed_at: Instant, + last_accessed_at: Instant, +} + +#[derive(Default)] +struct BuildFlight { + result: Mutex>>, + completed: Condvar, +} + +impl BuildFlight { + fn wait(&self) -> Result<(), String> { + let mut result = self.result.lock().unwrap(); + while result.is_none() { + result = self.completed.wait(result).unwrap(); + } + result.clone().unwrap() + } + + fn finish(&self, result: Result<(), String>) { + *self.result.lock().unwrap() = Some(result); + self.completed.notify_all(); + } +} + +struct CacheSlot { + generation: u64, + cached: Option, + in_flight: Option>, + last_accessed_at: Instant, +} + +impl CacheSlot { + fn new() -> Self { + Self { + generation: 0, + cached: None, + in_flight: None, + last_accessed_at: Instant::now(), + } + } +} + +#[derive(Default)] +struct CacheState { + slots: HashMap, +} + +enum CacheAction { + Return(Arc<[FileEntry]>), + Wait(Arc), + Build { + flight: Arc, + generation: u64, + }, +} + +/// Coordinates file-path indexes for every open workspace. +/// +/// A slot is keyed by both workspace root and exclusion policy. Equivalent +/// callers share one build. Invalidating a root bumps its generation so a +/// build that started before a file change can never repopulate the cache. +pub(super) struct FilePathIndexCache { + state: Mutex, + safety_ttl: Duration, + max_cached_indexes: usize, +} + +impl FilePathIndexCache { + pub(super) fn new(safety_ttl: Duration, max_cached_indexes: usize) -> Self { + Self { + state: Mutex::new(CacheState::default()), + safety_ttl, + max_cached_indexes, + } + } + + pub(super) fn get_or_build( + &self, + root_path: &str, + exclude_dirs: &[String], + build: F, + ) -> Result, String> + where + F: Fn() -> Vec, + { + let key = FileIndexKey::new(root_path, exclude_dirs); + + loop { + let action = { + let mut state = self.state.lock().unwrap(); + self.prune_locked(&mut state); + + let slot = state + .slots + .entry(key.clone()) + .or_insert_with(CacheSlot::new); + slot.last_accessed_at = Instant::now(); + + if let Some(cached) = slot.cached.as_mut() { + if cached.indexed_at.elapsed() < self.safety_ttl { + cached.last_accessed_at = Instant::now(); + CacheAction::Return(Arc::clone(&cached.entries)) + } else if let Some(flight) = slot.in_flight.as_ref() { + CacheAction::Wait(Arc::clone(flight)) + } else { + slot.cached = None; + let flight = Arc::new(BuildFlight::default()); + slot.in_flight = Some(Arc::clone(&flight)); + CacheAction::Build { + flight, + generation: slot.generation, + } + } + } else if let Some(flight) = slot.in_flight.as_ref() { + CacheAction::Wait(Arc::clone(flight)) + } else { + let flight = Arc::new(BuildFlight::default()); + slot.in_flight = Some(Arc::clone(&flight)); + CacheAction::Build { + flight, + generation: slot.generation, + } + } + }; + + match action { + CacheAction::Return(entries) => return Ok(entries), + CacheAction::Wait(flight) => { + flight.wait()?; + } + CacheAction::Build { flight, generation } => { + let build_result = catch_unwind(AssertUnwindSafe(&build)); + let entries = match build_result { + Ok(entries) => Arc::<[FileEntry]>::from(entries), + Err(_) => { + let error = format!("File index build panicked for {root_path}"); + self.finish_failed_build(&key, &flight, error.clone()); + return Err(error); + } + }; + + let accepted = { + let mut state = self.state.lock().unwrap(); + let Some(slot) = state.slots.get_mut(&key) else { + flight.finish(Ok(())); + continue; + }; + + let owns_flight = slot + .in_flight + .as_ref() + .is_some_and(|current| Arc::ptr_eq(current, &flight)); + if owns_flight { + slot.in_flight = None; + } + + if owns_flight && slot.generation == generation { + let now = Instant::now(); + slot.cached = Some(CachedIndex { + entries: Arc::clone(&entries), + indexed_at: now, + last_accessed_at: now, + }); + slot.last_accessed_at = now; + self.prune_locked(&mut state); + true + } else { + false + } + }; + + flight.finish(Ok(())); + if accepted { + return Ok(entries); + } + // A file change or explicit clear superseded this build. + // Loop so the caller receives a generation-current index. + } + } + } + } + + pub(super) fn invalidate_root(&self, root_path: &str) { + let mut state = self.state.lock().unwrap(); + for (key, slot) in &mut state.slots { + if key.root_path == root_path { + slot.generation = slot.generation.wrapping_add(1); + slot.cached = None; + slot.last_accessed_at = Instant::now(); + } + } + state + .slots + .retain(|_, slot| slot.cached.is_some() || slot.in_flight.is_some()); + } + + pub(super) fn clear(&self) { + let mut state = self.state.lock().unwrap(); + for slot in state.slots.values_mut() { + slot.generation = slot.generation.wrapping_add(1); + slot.cached = None; + } + state.slots.retain(|_, slot| slot.in_flight.is_some()); + } + + fn finish_failed_build(&self, key: &FileIndexKey, flight: &Arc, error: String) { + let mut state = self.state.lock().unwrap(); + if let Some(slot) = state.slots.get_mut(key) { + if slot + .in_flight + .as_ref() + .is_some_and(|current| Arc::ptr_eq(current, flight)) + { + slot.in_flight = None; + } + } + state + .slots + .retain(|_, slot| slot.cached.is_some() || slot.in_flight.is_some()); + drop(state); + flight.finish(Err(error)); + } + + fn prune_locked(&self, state: &mut CacheState) { + state.slots.retain(|_, slot| { + slot.in_flight.is_some() + || slot + .cached + .as_ref() + .is_some_and(|cached| cached.indexed_at.elapsed() < self.safety_ttl) + }); + + while state + .slots + .values() + .filter(|slot| slot.cached.is_some()) + .count() + > self.max_cached_indexes + { + let Some(oldest_key) = state + .slots + .iter() + .filter(|(_, slot)| slot.in_flight.is_none() && slot.cached.is_some()) + .min_by_key(|(_, slot)| slot.last_accessed_at) + .map(|(key, _)| key.clone()) + else { + break; + }; + state.slots.remove(&oldest_key); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Barrier; + use std::thread; + + fn entry(name: &str) -> FileEntry { + FileEntry { + path: format!("/repo/{name}"), + filename: name.to_string(), + is_dir: false, + } + } + + #[test] + fn equivalent_concurrent_requests_share_one_build() { + let cache = Arc::new(FilePathIndexCache::new(Duration::from_secs(60), 4)); + let build_count = Arc::new(AtomicUsize::new(0)); + let start = Arc::new(Barrier::new(8)); + + let threads: Vec<_> = (0..8) + .map(|_| { + let cache = Arc::clone(&cache); + let build_count = Arc::clone(&build_count); + let start = Arc::clone(&start); + thread::spawn(move || { + start.wait(); + cache + .get_or_build("/repo", &["target".to_string()], || { + build_count.fetch_add(1, Ordering::SeqCst); + thread::sleep(Duration::from_millis(40)); + vec![entry("main.rs")] + }) + .unwrap() + }) + }) + .collect(); + + for handle in threads { + assert_eq!(handle.join().unwrap().len(), 1); + } + assert_eq!(build_count.load(Ordering::SeqCst), 1); + } + + #[test] + fn invalidation_discards_an_in_flight_generation() { + let cache = Arc::new(FilePathIndexCache::new(Duration::from_secs(60), 4)); + let build_count = Arc::new(AtomicUsize::new(0)); + let first_started = Arc::new(Barrier::new(2)); + let resume_first = Arc::new(Barrier::new(2)); + + let worker = { + let cache = Arc::clone(&cache); + let build_count = Arc::clone(&build_count); + let first_started = Arc::clone(&first_started); + let resume_first = Arc::clone(&resume_first); + thread::spawn(move || { + cache + .get_or_build("/repo", &[], || { + let build_number = build_count.fetch_add(1, Ordering::SeqCst); + if build_number == 0 { + first_started.wait(); + resume_first.wait(); + } + vec![entry("main.rs")] + }) + .unwrap() + }) + }; + + first_started.wait(); + cache.invalidate_root("/repo"); + resume_first.wait(); + + assert_eq!(worker.join().unwrap().len(), 1); + assert_eq!(build_count.load(Ordering::SeqCst), 2); + } + + #[test] + fn exclusion_policy_is_part_of_the_cache_key() { + let cache = FilePathIndexCache::new(Duration::from_secs(60), 4); + let build_count = AtomicUsize::new(0); + + cache + .get_or_build("/repo", &["target".to_string()], || { + build_count.fetch_add(1, Ordering::SeqCst); + vec![entry("first")] + }) + .unwrap(); + cache + .get_or_build("/repo", &["node_modules".to_string()], || { + build_count.fetch_add(1, Ordering::SeqCst); + vec![entry("second")] + }) + .unwrap(); + + assert_eq!(build_count.load(Ordering::SeqCst), 2); + } + + #[test] + fn failed_owner_releases_waiters_and_allows_recovery() { + let cache = FilePathIndexCache::new(Duration::from_secs(60), 4); + let failed = cache.get_or_build("/repo", &[], || panic!("boom")); + assert!(failed.is_err()); + + let recovered = cache + .get_or_build("/repo", &[], || vec![entry("recovered")]) + .unwrap(); + assert_eq!(recovered[0].filename, "recovered"); + } +} diff --git a/src-tauri/crates/search/src/tests/file_tests.rs b/src-tauri/crates/search/src/tests/file_tests.rs index 991865e195..4e43596f7c 100644 --- a/src-tauri/crates/search/src/tests/file_tests.rs +++ b/src-tauri/crates/search/src/tests/file_tests.rs @@ -1,47 +1,6 @@ -use std::collections::HashMap; -use std::sync::Arc; +use app_utils::testing::temp_dir_with_files; -use crate::file::{ - estimate_file_index_bytes, fuzzy_search, prune_file_index_cache, FileEntry, FileIndex, -}; - -#[test] -fn file_index_size_estimate_includes_paths_and_entry_storage() { - let entries = vec![FileEntry { - path: "C:/repo/src/main.rs".to_string(), - filename: "main.rs".to_string(), - is_dir: false, - }]; - - assert!( - estimate_file_index_bytes(&entries) - >= std::mem::size_of::() + entries[0].path.len() + entries[0].filename.len() - ); -} - -#[test] -fn file_index_cache_prunes_to_global_byte_budget() { - let now = std::time::SystemTime::now(); - let mut cache = HashMap::new(); - for index in 0..3 { - cache.insert( - format!("repo-{index}"), - FileIndex { - entries: Arc::new(Vec::new()), - _root_path: format!("repo-{index}"), - indexed_at: now - .checked_sub(std::time::Duration::from_secs(3 - index)) - .expect("test timestamp should be representable"), - estimated_bytes: 24 * 1024 * 1024, - }, - ); - } - - prune_file_index_cache(&mut cache); - - assert_eq!(cache.len(), 2); - assert!(!cache.contains_key("repo-0")); -} +use crate::file::{build_file_index, default_excluded_dirs, fuzzy_search, FileEntry}; #[test] fn test_fuzzy_matching() { @@ -70,3 +29,50 @@ fn test_fuzzy_matching() { // "btn" should match "Button" better than others assert_eq!(results[0].0.filename, "Button.tsx"); } + +#[test] +fn file_index_skips_runtime_worktrees_but_keeps_user_orgii_files() { + let (_dir, root) = temp_dir_with_files(&[ + ("src/main.rs", "fn main() {}"), + (".env", "SECRET=not-a-real-secret"), + (".orgii/skills/example/SKILL.md", "# Example"), + (".orgii/worktrees/session-a/generated.rs", "generated"), + (".worktrees/session-b/generated.rs", "generated"), + ]); + + let entries = build_file_index(root.to_str().unwrap(), &default_excluded_dirs()); + let paths: Vec<_> = entries + .iter() + .filter_map(|entry| { + std::path::Path::new(&entry.path) + .strip_prefix(&root) + .ok() + .map(|path| path.to_string_lossy().to_string()) + }) + .collect(); + + assert!(paths.contains(&"src/main.rs".to_string())); + assert!(paths.contains(&".env".to_string())); + assert!(paths.contains(&".orgii/skills/example/SKILL.md".to_string())); + assert!(!paths + .iter() + .any(|path| path.starts_with(".orgii/worktrees"))); + assert!(!paths.iter().any(|path| path.starts_with(".worktrees"))); +} + +#[test] +fn fuzzy_search_honors_zero_and_top_k_limits() { + let entries: Vec<_> = (0..100) + .map(|index| FileEntry { + path: format!("src/component-{index}.tsx"), + filename: format!("component-{index}.tsx"), + is_dir: false, + }) + .collect(); + + assert!(fuzzy_search(&entries, "component", 0, None).is_empty()); + + let results = fuzzy_search(&entries, "component", 5, None); + assert_eq!(results.len(), 5); + assert!(results.windows(2).all(|pair| pair[0].1 >= pair[1].1)); +} diff --git a/src-tauri/src/commands/handler_list.inc b/src-tauri/src/commands/handler_list.inc index 7d43f057a4..402d529fab 100644 --- a/src-tauri/src/commands/handler_list.inc +++ b/src-tauri/src/commands/handler_list.inc @@ -223,6 +223,7 @@ search::file::search_files_fuzzy, search::file::index_project_files, search::file::prewarm_file_index, search::file::clear_file_index_cache, +search::file::invalidate_file_index_cache, search::file::find_files_by_extension, // Code search commands search::code::commands::search_code_regex, diff --git a/src/api/http/project/cache.test.ts b/src/api/http/project/cache.test.ts index 3cec15e838..4023f6e5fb 100644 --- a/src/api/http/project/cache.test.ts +++ b/src/api/http/project/cache.test.ts @@ -27,6 +27,24 @@ describe("project read cache invalidation fencing", () => { ]); }); + it("can deduplicate only the active request without caching its result", async () => { + const fetcher = vi + .fn<() => Promise>() + .mockResolvedValueOnce("first") + .mockResolvedValueOnce("second"); + + const first = cachedRead("project:filtered", fetcher, { maxAgeMs: 0 }); + const joined = cachedRead("project:filtered", fetcher, { maxAgeMs: 0 }); + await expect(Promise.all([first, joined])).resolves.toEqual([ + "first", + "first", + ]); + await expect( + cachedRead("project:filtered", fetcher, { maxAgeMs: 0 }) + ).resolves.toBe("second"); + expect(fetcher).toHaveBeenCalledTimes(2); + }); + it("never lets a pre-invalidation Promise resurrect or return stale data", async () => { let resolveStale: ((value: string) => void) | undefined; const fetcher = vi diff --git a/src/api/http/project/cache.ts b/src/api/http/project/cache.ts index 1ae3348579..070d365369 100644 --- a/src/api/http/project/cache.ts +++ b/src/api/http/project/cache.ts @@ -66,11 +66,13 @@ function evictIfNeeded(): void { export async function cachedRead( cacheKey: string, - fetcher: () => Promise + fetcher: () => Promise, + options?: { maxAgeMs?: number } ): Promise { + const maxAgeMs = options?.maxAgeMs ?? CACHE_TTL_MS; const now = Date.now(); const existing = cache.get(cacheKey); - if (existing && now - existing.timestamp < CACHE_TTL_MS) { + if (maxAgeMs > 0 && existing && now - existing.timestamp < maxAgeMs) { return existing.data as T; } @@ -101,10 +103,12 @@ export async function cachedRead( // stale snapshot; converge the original waiter onto the post-change // read (or its already-running shared Promise) instead. if (inflight.get(cacheKey) === promise) inflight.delete(cacheKey); - return cachedRead(cacheKey, fetcher); + return cachedRead(cacheKey, fetcher, options); + } + if (maxAgeMs > 0) { + evictIfNeeded(); + cache.set(cacheKey, { data: result, timestamp: Date.now() }); } - evictIfNeeded(); - cache.set(cacheKey, { data: result, timestamp: Date.now() }); if (inflight.get(cacheKey) === promise) inflight.delete(cacheKey); return result; }) diff --git a/src/api/http/project/client.purge.test.ts b/src/api/http/project/client.purge.test.ts new file mode 100644 index 0000000000..eee106467a --- /dev/null +++ b/src/api/http/project/client.purge.test.ts @@ -0,0 +1,42 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { __TESTS_ONLY, purgeExpiredDeletedWorkItems } from "./client"; + +const { invokeMock } = vi.hoisted(() => ({ + invokeMock: vi.fn(), +})); + +vi.mock("@tauri-apps/api/core", () => ({ + invoke: invokeMock, +})); + +describe("expired work-item purge coordination", () => { + beforeEach(() => { + __TESTS_ONLY.resetPurgeCoordinator(); + invokeMock.mockReset(); + }); + + it("shares an active purge and throttles later filter refreshes", async () => { + invokeMock.mockResolvedValue(0); + + const first = purgeExpiredDeletedWorkItems("project-a"); + const joined = purgeExpiredDeletedWorkItems("project-a"); + await expect(Promise.all([first, joined])).resolves.toEqual([0, 0]); + await expect(purgeExpiredDeletedWorkItems("project-a")).resolves.toBe(0); + + expect(invokeMock).toHaveBeenCalledTimes(1); + }); + + it("releases a failed purge so the next request can retry", async () => { + invokeMock + .mockRejectedValueOnce(new Error("database busy")) + .mockResolvedValueOnce(0); + + await expect(purgeExpiredDeletedWorkItems("project-a")).rejects.toThrow( + "database busy" + ); + await expect(purgeExpiredDeletedWorkItems("project-a")).resolves.toBe(0); + + expect(invokeMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/api/http/project/client.ts b/src/api/http/project/client.ts index 260fd30993..34f2e649c8 100644 --- a/src/api/http/project/client.ts +++ b/src/api/http/project/client.ts @@ -52,6 +52,28 @@ import type { WorkspaceWorkItemsData, } from "./types"; +const PURGE_DELETED_ITEMS_MIN_INTERVAL_MS = 5 * 60 * 1_000; +const MAX_PURGE_PROJECTS = 50; + +interface PurgeState { + inFlight?: Promise; + lastRunAt?: number; +} + +const purgeStateByProject = new Map(); + +function getPurgeState(projectSlug: string): PurgeState { + const existing = purgeStateByProject.get(projectSlug); + if (existing) return existing; + if (purgeStateByProject.size >= MAX_PURGE_PROJECTS) { + const oldestKey = purgeStateByProject.keys().next().value; + if (oldestKey) purgeStateByProject.delete(oldestKey); + } + const state: PurgeState = {}; + purgeStateByProject.set(projectSlug, state); + return state; +} + // ============================================ // Init / discovery // ============================================ @@ -451,18 +473,28 @@ export async function readWorkItemsViewData( const { statusFilter, searchQuery, view } = options ?? {}; const scopePayload = scopeInvokePayload(options); const scopeSegment = scopeCacheSegment(options); + const normalizedSearchQuery = searchQuery?.trim() || undefined; const hasFilters = - (statusFilter && statusFilter !== "all") || - (searchQuery && searchQuery.trim()); + (statusFilter && statusFilter !== "all") || normalizedSearchQuery; if (hasFilters) { - return invoke("project_read_work_items_view_data", { - projectSlug, - ...scopePayload, - statusFilter: statusFilter ?? null, - searchQuery: searchQuery ?? null, - view: view ?? null, - }); + const filterSegment = JSON.stringify([ + statusFilter ?? null, + normalizedSearchQuery ?? null, + view ?? null, + ]); + return cachedRead( + `${projectSlug}:workitems-view:${scopeSegment}:${filterSegment}`, + () => + invoke("project_read_work_items_view_data", { + projectSlug, + ...scopePayload, + statusFilter: statusFilter ?? null, + searchQuery: normalizedSearchQuery ?? null, + view: view ?? null, + }), + { maxAgeMs: 0 } + ); } return cachedRead( @@ -657,14 +689,36 @@ export async function restoreWorkItem( export async function purgeExpiredDeletedWorkItems( projectSlug: string ): Promise { - const result = await invoke( - "project_purge_expired_deleted_work_items", - { projectSlug } - ); - invalidateCache(projectSlug); - return result; + const state = getPurgeState(projectSlug); + if (state.inFlight) return state.inFlight; + if ( + state.lastRunAt !== undefined && + Date.now() - state.lastRunAt < PURGE_DELETED_ITEMS_MIN_INTERVAL_MS + ) { + return 0; + } + + const request = invoke("project_purge_expired_deleted_work_items", { + projectSlug, + }).then((result) => { + state.lastRunAt = Date.now(); + if (result > 0) invalidateCache(projectSlug); + return result; + }); + state.inFlight = request; + const release = () => { + if (state.inFlight === request) state.inFlight = undefined; + }; + void request.then(release, release); + return request; } +export const __TESTS_ONLY = { + resetPurgeCoordinator(): void { + purgeStateByProject.clear(); + }, +}; + /** * Atomic partial update; the Rust handler holds an `IMMEDIATE` * transaction across the read-modify-write so concurrent edits diff --git a/src/api/tauri/repo/__tests__/repoListCoordinator.test.ts b/src/api/tauri/repo/__tests__/repoListCoordinator.test.ts new file mode 100644 index 0000000000..44aafec595 --- /dev/null +++ b/src/api/tauri/repo/__tests__/repoListCoordinator.test.ts @@ -0,0 +1,99 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { __TESTS_ONLY, deleteRepo, getRepos } from "@src/api/tauri/repo"; + +const { invokeMock } = vi.hoisted(() => ({ + invokeMock: vi.fn(), +})); + +vi.mock("@tauri-apps/api/core", () => ({ + invoke: invokeMock, +})); + +function backendRepo(id: string) { + return { + id, + repo_id: id, + name: id, + path: `/repos/${id}`, + }; +} + +describe("repository list coordinator", () => { + beforeEach(() => { + __TESTS_ONLY.resetRepoListCoordinator(); + invokeMock.mockReset(); + }); + + it("shares one list request between concurrent consumers", async () => { + invokeMock.mockResolvedValue([backendRepo("one")]); + + const [first, second] = await Promise.all([getRepos(), getRepos()]); + + expect(invokeMock).toHaveBeenCalledTimes(1); + expect(first).toEqual(second); + }); + + it("runs one trailing request when force refresh arrives in flight", async () => { + let releaseFirst!: (repos: ReturnType[]) => void; + invokeMock + .mockImplementationOnce( + () => + new Promise((resolve) => { + releaseFirst = resolve; + }) + ) + .mockResolvedValueOnce([backendRepo("fresh")]); + + const initial = getRepos(); + const forced = getRepos({ forceRefresh: true }); + releaseFirst([backendRepo("stale")]); + + const [initialResult, forcedResult] = await Promise.all([initial, forced]); + + expect(invokeMock).toHaveBeenCalledTimes(2); + expect(initialResult.data.repos[0]?.repo_id).toBe("fresh"); + expect(forcedResult.data.repos[0]?.repo_id).toBe("fresh"); + }); + + it("refreshes an active list after a repository mutation", async () => { + let releaseFirst!: (repos: ReturnType[]) => void; + invokeMock.mockImplementation((command: string) => { + if (command === "server_delete_repo") return Promise.resolve(true); + if ( + invokeMock.mock.calls.filter(([name]) => name === "server_list_repos") + .length === 1 + ) { + return new Promise((resolve) => { + releaseFirst = resolve; + }); + } + return Promise.resolve([backendRepo("remaining")]); + }); + + const listing = getRepos(); + await deleteRepo("removed"); + releaseFirst([backendRepo("removed"), backendRepo("remaining")]); + const result = await listing; + + expect( + invokeMock.mock.calls.filter(([name]) => name === "server_list_repos") + ).toHaveLength(2); + expect(result.data.repos.map((repo) => repo.repo_id)).toEqual([ + "remaining", + ]); + }); + + it("releases a failed list request so a later load can retry", async () => { + invokeMock + .mockRejectedValueOnce(new Error("backend unavailable")) + .mockResolvedValueOnce([backendRepo("recovered")]); + + await expect(getRepos()).rejects.toThrow("backend unavailable"); + await expect(getRepos()).resolves.toMatchObject({ + data: { repos: [{ repo_id: "recovered" }] }, + }); + + expect(invokeMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/api/tauri/repo/index.ts b/src/api/tauri/repo/index.ts index 07888b7659..17690feed6 100644 --- a/src/api/tauri/repo/index.ts +++ b/src/api/tauri/repo/index.ts @@ -36,12 +36,31 @@ function wrapResponse(data: T) { return { data, status: 0 }; } +interface RepoListResponse { + data: RepoList; + status: number; +} + +interface RepoListFlight { + forceRefresh: boolean; + generation: number; + promise: Promise; +} + +let repoListGeneration = 0; +let repoListFlight: RepoListFlight | undefined; +let repoListForcePending = false; + +function markRepoListChanged(forceRefresh = false): void { + repoListGeneration += 1; + repoListForcePending ||= forceRefresh; +} + // ============================================ // Repository CRUD (via Tauri commands) // ============================================ -/** Get current user's repository list */ -export async function getRepos() { +async function fetchRepos(): Promise { const repos = await invokeTauri< Array<{ id: string; @@ -66,6 +85,72 @@ export async function getRepos() { return wrapResponse({ repos: mapped }); } +/** + * Get the current repository list with one shared IPC request. + * + * A force request arriving behind a normal load, or a mutation completing + * during a load, advances the generation. The old response is awaited but not + * returned; all callers then share one trailing request. + */ +export async function getRepos(options?: { + forceRefresh?: boolean; +}): Promise { + const forceRefresh = options?.forceRefresh ?? false; + const current = repoListFlight; + + if ( + forceRefresh && + current && + !current.forceRefresh && + current.generation === repoListGeneration + ) { + markRepoListChanged(true); + } + + if (current) { + try { + const response = await current.promise; + if ( + current.generation === repoListGeneration && + (!forceRefresh || current.forceRefresh) + ) { + return response; + } + } catch (error) { + if (current.generation === repoListGeneration) throw error; + } + return getRepos({ forceRefresh }); + } + + const effectiveForceRefresh = forceRefresh || repoListForcePending; + repoListForcePending = false; + const generation = repoListGeneration; + const promise = fetchRepos(); + const flight: RepoListFlight = { + forceRefresh: effectiveForceRefresh, + generation, + promise, + }; + repoListFlight = flight; + const release = () => { + if (repoListFlight === flight) repoListFlight = undefined; + }; + void promise.then(release, release); + + try { + const response = await promise; + if (generation !== repoListGeneration) { + return getRepos(); + } + return response; + } catch (error) { + if (generation !== repoListGeneration) { + return getRepos(); + } + throw error; + } +} + /** Get repository by ID (path) */ export async function getRepoById(repoId: string) { const result = await invokeTauri<{ @@ -88,6 +173,7 @@ export async function getRepoById(repoId: string) { /** Delete / unwatch repository */ export async function deleteRepo(repoId: string) { await invokeTauri("server_delete_repo", { repoId }); + markRepoListChanged(); return wrapResponse(null); } @@ -97,6 +183,7 @@ export async function updateRepoVisibility( visibility: "public" | "private" ) { await invokeTauri("server_update_repo_visibility", { path, visibility }); + markRepoListChanged(); } /** Check GitHub repo visibility via backend (no CORS issues). Returns "public", "private", or null. */ @@ -124,6 +211,7 @@ export async function importLocalRepo(data: { fs_path: string }) { path: string; kind?: string; }>("server_import_repo", { path: data.fs_path }); + markRepoListChanged(); const repo: Repo = { repo_id: result.repo_id || result.id, user_id: "", @@ -149,6 +237,7 @@ export async function createFromGithub(data: { url: data.github_url, targetDir: data.fs_path, }); + markRepoListChanged(); const repo: Repo = { repo_id: result.repo_id || result.id, user_id: "", @@ -176,6 +265,7 @@ export async function createEmptyRepo(data: { path: data.fs_path, name: data.name, }); + markRepoListChanged(); const repo: Repo = { repo_id: result.repo_id || result.id, user_id: "", @@ -195,6 +285,7 @@ export async function importWorkFolder(data: { fs_path: string }) { path: string; kind: string; }>("server_import_folder", { path: data.fs_path }); + markRepoListChanged(); const repo: Repo = { repo_id: result.repo_id || result.id, user_id: "", @@ -220,6 +311,7 @@ export async function createWorkFolder(data: { path: data.fs_path, name: data.name, }); + markRepoListChanged(); const repo: Repo = { repo_id: result.repo_id || result.id, user_id: "", @@ -293,4 +385,12 @@ export const repoApi = { detectIDEs, }; +export const __TESTS_ONLY = { + resetRepoListCoordinator() { + repoListGeneration = 0; + repoListFlight = undefined; + repoListForcePending = false; + }, +}; + export default repoApi; diff --git a/src/contexts/git/GitStatusContext/GitStatusProvider.tsx b/src/contexts/git/GitStatusContext/GitStatusProvider.tsx index 091552808a..e063a0304c 100644 --- a/src/contexts/git/GitStatusContext/GitStatusProvider.tsx +++ b/src/contexts/git/GitStatusContext/GitStatusProvider.tsx @@ -141,6 +141,19 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({ return currentRepo?.path || currentRepo?.fs_uri; }, [currentRepo]); + const resolveRepoPath = useCallback( + (repoId: string): string | undefined => { + const repo = repoMap.get(repoId); + if (repo?.path || repo?.fs_uri) return repo.path || repo.fs_uri; + + const folder = workspaceFolders.find( + (candidate) => candidate.id === repoId || candidate.repoId === repoId + ); + return folder?.path; + }, + [repoMap, workspaceFolders] + ); + // ============================================ // Watcher Registration Hook // ============================================ @@ -233,6 +246,7 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({ setGitStatusAtom, setGitSuggestedActionAtom, setGitOperation, + resolveRepoPath, }); // ============================================ diff --git a/src/contexts/git/GitStatusContext/hooks/__tests__/TEST_CASES.md b/src/contexts/git/GitStatusContext/hooks/__tests__/TEST_CASES.md new file mode 100644 index 0000000000..28ead01a56 --- /dev/null +++ b/src/contexts/git/GitStatusContext/hooks/__tests__/TEST_CASES.md @@ -0,0 +1,34 @@ +# File index invalidation test cases + +## Preconditions + +- Git watcher events identify a repository with `repo_id`. +- File-path index invalidation is a cheap state transition; it does not scan. +- Content-only modifications do not change the indexed path set. + +## Happy path + +- Created, deleted, renamed, or unknown file events schedule invalidation. +- Aggregate `repo:changed` events invalidate only when `change_type` is `files`. +- Multiple events for one repository inside 250 ms produce one invalidation. +- Simultaneous events for different repositories invalidate each root once. + +## Edge cases + +- `modified` events are ignored because filenames and paths are unchanged. +- Disposing the listener clears pending invalidations. +- Empty paths are not scheduled. + +## Error path + +- A rejected Tauri invalidation request is reported through the supplied error callback and does not create an unhandled rejection. + +## Accessibility + +- Not applicable: this lifecycle change has no rendered UI or input behavior. + +## Acceptance criteria + +- No timer causes indexing or repeated background scans. +- A watcher burst produces at most one cheap invalidation call per root. +- Listener teardown leaves no pending timer. diff --git a/src/contexts/git/GitStatusContext/hooks/__tests__/fileIndexInvalidation.test.ts b/src/contexts/git/GitStatusContext/hooks/__tests__/fileIndexInvalidation.test.ts new file mode 100644 index 0000000000..b1c28a4c60 --- /dev/null +++ b/src/contexts/git/GitStatusContext/hooks/__tests__/fileIndexInvalidation.test.ts @@ -0,0 +1,75 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + createFileIndexInvalidationScheduler, + fileChangeInvalidatesPathIndex, + repoChangeInvalidatesPathIndex, +} from "../fileIndexInvalidation"; + +describe("file index invalidation", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("coalesces a burst into one invalidation per workspace root", async () => { + const invalidate = vi.fn().mockResolvedValue(undefined); + const scheduler = createFileIndexInvalidationScheduler(invalidate, 250); + + scheduler.schedule("/repo-a"); + scheduler.schedule("/repo-a"); + scheduler.schedule("/repo-b"); + vi.advanceTimersByTime(249); + expect(invalidate).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(1); + await Promise.resolve(); + expect(invalidate).toHaveBeenCalledTimes(2); + expect(invalidate).toHaveBeenCalledWith("/repo-a"); + expect(invalidate).toHaveBeenCalledWith("/repo-b"); + }); + + it("drops pending work after disposal", () => { + const invalidate = vi.fn().mockResolvedValue(undefined); + const scheduler = createFileIndexInvalidationScheduler(invalidate, 250); + + scheduler.schedule("/repo-a"); + scheduler.dispose(); + vi.advanceTimersByTime(500); + + expect(invalidate).not.toHaveBeenCalled(); + }); + + it("reports asynchronous invalidation failures", async () => { + const error = new Error("IPC unavailable"); + const onError = vi.fn(); + const scheduler = createFileIndexInvalidationScheduler( + vi.fn().mockRejectedValue(error), + 1, + onError + ); + + scheduler.schedule("/repo-a"); + await vi.advanceTimersByTimeAsync(1); + + expect(onError).toHaveBeenCalledWith(error); + }); + + it("ignores content-only modifications", () => { + expect(fileChangeInvalidatesPathIndex("modified")).toBe(false); + expect(fileChangeInvalidatesPathIndex("created")).toBe(true); + expect(fileChangeInvalidatesPathIndex("deleted")).toBe(true); + expect(fileChangeInvalidatesPathIndex("renamed")).toBe(true); + expect(fileChangeInvalidatesPathIndex(undefined)).toBe(true); + }); + + it("only invalidates for aggregate filesystem changes", () => { + expect(repoChangeInvalidatesPathIndex("files")).toBe(true); + expect(repoChangeInvalidatesPathIndex("git_meta")).toBe(false); + expect(repoChangeInvalidatesPathIndex("branch")).toBe(false); + expect(repoChangeInvalidatesPathIndex(undefined)).toBe(false); + }); +}); diff --git a/src/contexts/git/GitStatusContext/hooks/fileIndexInvalidation.ts b/src/contexts/git/GitStatusContext/hooks/fileIndexInvalidation.ts new file mode 100644 index 0000000000..b15584fd48 --- /dev/null +++ b/src/contexts/git/GitStatusContext/hooks/fileIndexInvalidation.ts @@ -0,0 +1,52 @@ +export interface FileIndexInvalidationScheduler { + schedule(rootPath: string): void; + dispose(): void; +} + +/** + * Coalesces file-create/delete/rename bursts into one invalidation per root. + * Invalidating is deliberately cheap: it marks state stale but never scans. + */ +export function createFileIndexInvalidationScheduler( + invalidate: (rootPath: string) => Promise, + delayMs = 250, + onError: (error: unknown) => void = () => undefined +): FileIndexInvalidationScheduler { + const pendingRoots = new Set(); + let timer: ReturnType | null = null; + let disposed = false; + + const flush = () => { + timer = null; + const roots = [...pendingRoots]; + pendingRoots.clear(); + + for (const rootPath of roots) { + void invalidate(rootPath).catch(onError); + } + }; + + return { + schedule(rootPath) { + if (disposed || !rootPath) return; + pendingRoots.add(rootPath); + if (timer) return; + timer = setTimeout(flush, delayMs); + }, + dispose() { + disposed = true; + pendingRoots.clear(); + if (timer) clearTimeout(timer); + timer = null; + }, + }; +} + +/** Content-only modifications do not change a file-path index. */ +export function fileChangeInvalidatesPathIndex(kind: unknown): boolean { + return typeof kind !== "string" || kind !== "modified"; +} + +export function repoChangeInvalidatesPathIndex(changeType: unknown): boolean { + return changeType === "files"; +} diff --git a/src/contexts/git/GitStatusContext/hooks/useGitEventListeners.ts b/src/contexts/git/GitStatusContext/hooks/useGitEventListeners.ts index 45677b146d..3650411d25 100644 --- a/src/contexts/git/GitStatusContext/hooks/useGitEventListeners.ts +++ b/src/contexts/git/GitStatusContext/hooks/useGitEventListeners.ts @@ -18,8 +18,14 @@ import type { } from "@src/types/session/steps"; import { decodeOctalPath } from "@src/util/file/pathUtils"; import { computeSuggestedAction } from "@src/util/git/computeSuggestedAction"; +import { invalidateFileIndexCache } from "@src/util/platform/tauri/fileSearch"; import type { GitStatusRefs } from "../types"; +import { + createFileIndexInvalidationScheduler, + fileChangeInvalidatesPathIndex, + repoChangeInvalidatesPathIndex, +} from "./fileIndexInvalidation"; const log = createLogger("GitStatusContext"); @@ -38,6 +44,7 @@ interface UseGitEventListenersOptions { details: string; timestamp: number; }) => void; + resolveRepoPath: (repoId: string) => string | undefined; } export function useGitEventListeners({ @@ -48,6 +55,7 @@ export function useGitEventListeners({ setGitStatusAtom, setGitSuggestedActionAtom, setGitOperation, + resolveRepoPath, }: UseGitEventListenersOptions): void { const { currentRepoIdRef, gitStatusRef } = refs; @@ -60,6 +68,7 @@ export function useGitEventListeners({ const setGitStatusAtomRef = useRef(setGitStatusAtom); const setGitSuggestedActionAtomRef = useRef(setGitSuggestedActionAtom); const setGitOperationRef = useRef(setGitOperation); + const resolveRepoPathRef = useRef(resolveRepoPath); useEffect(() => { setGitStatusRef.current = setGitStatus; }, [setGitStatus]); @@ -75,11 +84,20 @@ export function useGitEventListeners({ useEffect(() => { setGitOperationRef.current = setGitOperation; }, [setGitOperation]); + useEffect(() => { + resolveRepoPathRef.current = resolveRepoPath; + }, [resolveRepoPath]); useEffect(() => { if (!selectedRepoId) return; const cleanupFns: (() => void)[] = []; + const fileIndexInvalidation = createFileIndexInvalidationScheduler( + invalidateFileIndexCache, + 250, + (error) => + log.warn("[GitStatusContext] File index invalidation failed:", error) + ); const setupListeners = () => { try { @@ -209,9 +227,44 @@ export function useGitEventListeners({ }); cleanupFns.push(unsubscribeStatus); - // Listen to file:changed for file changes - const unsubscribeChanged = ws.on("file:changed", (_data) => { - // Status will arrive via repo:status_updated event from debouncer + // The repository watcher emits this aggregate event for every real + // filesystem burst. It is the canonical path-index invalidation source. + const unsubscribeRepoChanged = ws.on("repo:changed", (data) => { + const event = data as { + repo_id?: string; + change_type?: string; + }; + if ( + !event.repo_id || + !repoChangeInvalidatesPathIndex(event.change_type) + ) { + return; + } + + const rootPath = resolveRepoPathRef.current(event.repo_id); + if (rootPath) fileIndexInvalidation.schedule(rootPath); + }); + cleanupFns.push(unsubscribeRepoChanged); + + // Retain compatibility with granular file events from future or + // alternate watcher producers. + const unsubscribeChanged = ws.on("file:changed", (data) => { + const event = data as { + repo_id?: string; + kind?: string; + files?: Array<{ kind?: string }>; + }; + if (!event.repo_id) return; + + const containsPathChange = event.files + ? event.files.some((file) => + fileChangeInvalidatesPathIndex(file.kind) + ) + : fileChangeInvalidatesPathIndex(event.kind); + if (!containsPathChange) return; + + const rootPath = resolveRepoPathRef.current(event.repo_id); + if (rootPath) fileIndexInvalidation.schedule(rootPath); }); cleanupFns.push(unsubscribeChanged); @@ -247,6 +300,7 @@ export function useGitEventListeners({ setupListeners(); return () => { + fileIndexInvalidation.dispose(); cleanupFns.forEach((fn) => fn()); }; }, [ diff --git a/src/engines/BrowserCore/BrowserCore.webviewRetention.test.ts b/src/engines/BrowserCore/BrowserCore.webviewRetention.test.ts new file mode 100644 index 0000000000..d828c189c0 --- /dev/null +++ b/src/engines/BrowserCore/BrowserCore.webviewRetention.test.ts @@ -0,0 +1,170 @@ +// @vitest-environment jsdom +import { act, createElement } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { UseBrowserStateReturn } from "./hooks/useBrowserState"; +import { + BrowserCore, + MAX_RETAINED_BROWSER_WEBVIEWS, + selectRetainedBrowserSessionIds, +} from "./index"; +import type { BrowserSession } from "./types"; + +vi.mock("jotai", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useAtomValue: () => false, + }; +}); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("./BrowserSessionWebview", () => ({ + default: ({ + session, + isActive, + }: { + session: BrowserSession; + isActive: boolean; + }) => + createElement("div", { + "data-browser-webview-session": session.id, + "data-active": String(isActive), + }), +})); + +const reactActEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; +}; + +function session( + id: string, + url = `https://${id}.example.com` +): BrowserSession { + return { + id, + title: id, + url, + history: url ? [url] : [], + historyIndex: url ? 0 : -1, + historyEntries: [], + isLoading: false, + error: null, + incognito: false, + }; +} + +function browserState( + sessions: BrowserSession[], + activeSessionId: string +): UseBrowserStateReturn { + return { + sessions, + activeSessionId, + activeSession: sessions.find((item) => item.id === activeSessionId), + addSession: vi.fn(), + closeSession: vi.fn(), + setActiveSession: vi.fn(), + updateSession: vi.fn(), + }; +} + +describe("BrowserCore retained native WebViews", () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + reactActEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + (window as unknown as Record).__TAURI_INTERNALS__ = {}; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + Reflect.deleteProperty( + window as unknown as Record, + "__TAURI_INTERNALS__" + ); + Reflect.deleteProperty(reactActEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + vi.restoreAllMocks(); + }); + + function renderWith( + sessions: BrowserSession[], + activeSessionId: string + ): void { + act(() => { + root.render( + createElement(BrowserCore, { + browserState: browserState(sessions, activeSessionId), + }) + ); + }); + } + + function mountedSessionIds(): string[] { + return Array.from( + container.querySelectorAll("[data-browser-webview-session]") + ).map((element) => element.getAttribute("data-browser-webview-session")!); + } + + it("keeps only the active and most recently active native WebViews mounted", () => { + const sessions = [session("a"), session("b"), session("c")]; + + renderWith(sessions, "a"); + expect(mountedSessionIds()).toEqual(["a"]); + + renderWith(sessions, "b"); + expect(mountedSessionIds()).toEqual(["a", "b"]); + + renderWith(sessions, "c"); + expect(mountedSessionIds()).toEqual(["b", "c"]); + + renderWith(sessions, "a"); + expect(mountedSessionIds()).toEqual(["a", "c"]); + expect(mountedSessionIds()).toHaveLength(MAX_RETAINED_BROWSER_WEBVIEWS); + }); + + it("does not mount restored background sessions or blank tabs eagerly", () => { + const sessions = [session("a"), session("blank", ""), session("c")]; + + renderWith(sessions, "blank"); + expect(mountedSessionIds()).toEqual([]); + + renderWith(sessions, "c"); + expect(mountedSessionIds()).toEqual(["c"]); + }); + + it("keeps the native mount count bounded across repeated session switches", () => { + const sessions = [session("a"), session("b"), session("c"), session("d")]; + + for (let index = 0; index < 50; index += 1) { + const activeSessionId = sessions[index % sessions.length].id; + renderWith(sessions, activeSessionId); + + expect(mountedSessionIds()).toContain(activeSessionId); + expect(mountedSessionIds().length).toBeLessThanOrEqual( + MAX_RETAINED_BROWSER_WEBVIEWS + ); + } + }); +}); + +describe("selectRetainedBrowserSessionIds", () => { + it("drops deleted and non-navigable sessions while preserving recency", () => { + expect( + selectRetainedBrowserSessionIds( + ["deleted", "a", "blank"], + [session("a"), session("blank", ""), session("b")], + "b" + ) + ).toEqual(["a", "b"]); + }); +}); diff --git a/src/engines/BrowserCore/index.tsx b/src/engines/BrowserCore/index.tsx index 3ebe00bd7c..b2d4fb1f06 100644 --- a/src/engines/BrowserCore/index.tsx +++ b/src/engines/BrowserCore/index.tsx @@ -36,12 +36,14 @@ import BrowserSessionWebview from "./BrowserSessionWebview"; import type { UseBrowserStateReturn } from "./hooks/useBrowserState"; import "./index.scss"; import { BROWSER_WEBVIEW_FRAME_ANCHOR_ATTRIBUTE } from "./nativeFrameAnchor"; +import type { BrowserSession } from "./types"; const log = createLogger("BrowserCore"); const ABOUT_BLANK_URL = "about:blank"; const SHOW_WEBVIEW_FRAME_ANCHOR = false; const EMBEDDED_BROWSER_WARNING_DELAY_MS = 3000; +export const MAX_RETAINED_BROWSER_WEBVIEWS = 2; const EMBEDDED_BROWSER_SENSITIVE_HOSTS = new Set([ "github.com", "www.github.com", @@ -66,6 +68,37 @@ function shouldShowEmbeddedBrowserFallback(url?: string): boolean { } } +export function selectRetainedBrowserSessionIds( + previousIds: readonly string[], + sessions: readonly BrowserSession[], + activeSessionId: string, + maxRetained = MAX_RETAINED_BROWSER_WEBVIEWS +): string[] { + if (maxRetained <= 0) return []; + + const navigableIds = new Set( + sessions + .filter((session) => !isBlankBrowserUrl(session.url)) + .map((session) => session.id) + ); + const next = previousIds.filter( + (sessionId) => navigableIds.has(sessionId) && sessionId !== activeSessionId + ); + + if (navigableIds.has(activeSessionId)) { + next.push(activeSessionId); + } + + return next.slice(-maxRetained); +} + +function sameIds(left: readonly string[], right: readonly string[]): boolean { + return ( + left.length === right.length && + left.every((sessionId, index) => sessionId === right[index]) + ); +} + // ============================================ // Props // ============================================ @@ -112,6 +145,29 @@ export const BrowserCore: React.FC = ({ }) => { const { t } = useTranslation(); const { sessions, activeSessionId, updateSession, addSession } = browserState; + const [retainedWebviewSessionIds, setRetainedWebviewSessionIds] = + React.useState([]); + const nextRetainedWebviewSessionIds = useMemo( + () => + selectRetainedBrowserSessionIds( + retainedWebviewSessionIds, + sessions, + activeSessionId + ), + [activeSessionId, retainedWebviewSessionIds, sessions] + ); + const retainedWebviewSessionIdSet = useMemo( + () => new Set(nextRetainedWebviewSessionIds), + [nextRetainedWebviewSessionIds] + ); + + React.useLayoutEffect(() => { + setRetainedWebviewSessionIds((previousIds) => + sameIds(previousIds, nextRetainedWebviewSessionIds) + ? previousIds + : nextRetainedWebviewSessionIds + ); + }, [nextRetainedWebviewSessionIds]); // Check if webviews should be blocked by overlays or station ownership. const isWebviewBlocked = useAtomValue(webviewBlockedAtom); @@ -282,17 +338,19 @@ export const BrowserCore: React.FC = ({ {/* Only the owning instance renders BrowserSessionWebview. */} {manageWebviews && - sessions.map((session) => ( - - ))} + sessions + .filter((session) => retainedWebviewSessionIdSet.has(session.id)) + .map((session) => ( + + ))} {/* Desktop-only notice */} {!isWebviewAvailable && ( diff --git a/src/features/BenchmarkPanel/index.tsx b/src/features/BenchmarkPanel/index.tsx index 5df7b7a2e7..bc5a2756ff 100644 --- a/src/features/BenchmarkPanel/index.tsx +++ b/src/features/BenchmarkPanel/index.tsx @@ -21,6 +21,10 @@ import TabPill from "@src/components/TabPill"; import { SURFACE_TOKENS } from "@src/config/surfaceTokens"; import BenchmarkTaskSelector from "@src/features/BenchmarkPanel/BenchmarkTaskSelector"; import { CodeMirrorEditor } from "@src/features/CodeMirror"; +import { + listBenchmarkTasksShared, + setBenchmarkAgentBatchStatusShared, +} from "@src/hooks/benchmark/benchmarkRequestCoordinator"; import { useBenchmarkAgentBatchRun } from "@src/hooks/benchmark/useBenchmarkAgentBatchRun"; import { useBenchmarkTasks } from "@src/hooks/benchmark/useBenchmarkTasks"; import { usePublishWorkstationTabHeader } from "@src/hooks/workStation"; @@ -170,7 +174,7 @@ export const BenchmarkPanel: React.FC = ({ setAddTasksLoading(true); setAddTasksError(null); try { - const rows = await benchmarkApi.listTasks({ + const rows = await listBenchmarkTasksShared({ kind: status.benchmarkKind, sourcePath: status.sourcePath, limit: BENCHMARK_TASK_LIST_LIMIT, @@ -310,6 +314,7 @@ export const BenchmarkPanel: React.FC = ({ action, taskIds, }); + setBenchmarkAgentBatchStatusShared(status); setBenchmarkBatchStatus(status); void loadSessions({ forceRefresh: true }); return true; @@ -418,6 +423,7 @@ export const BenchmarkPanel: React.FC = ({ batchId: batchStatus.batchId, evaluationMode: BENCHMARK_EVALUATION_MODE.LOCAL_DOCKER, }); + setBenchmarkAgentBatchStatusShared(status); setBenchmarkBatchStatus(status); const evaluatedCount = status.items.filter( (item) => item.evaluationStatus diff --git a/src/features/TaskKanban/hooks/__tests__/useSessionImpact.test.ts b/src/features/TaskKanban/hooks/__tests__/useSessionImpact.test.ts new file mode 100644 index 0000000000..e866238b52 --- /dev/null +++ b/src/features/TaskKanban/hooks/__tests__/useSessionImpact.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "vitest"; + +import type { CoreSessionSummary } from "@src/api/tauri/lineage"; +import type { Session } from "@src/store/session"; + +import { + impactFromSummaries, + sessionImpactRosterKey, +} from "../useSessionImpact"; + +function summary( + sessionId: string, + overrides: Partial = {} +): CoreSessionSummary { + return { + sessionId, + filesChanged: 4, + linesAdded: 40, + linesRemoved: 10, + relatedCommits: 2, + committedRatePercent: 50, + ...overrides, + } as CoreSessionSummary; +} + +function session(sessionId: string): Session { + return { session_id: sessionId } as Session; +} + +describe("impactFromSummaries", () => { + it("keeps only the sessions the board can render", () => { + const impact = impactFromSummaries( + [summary("visible"), summary("archived-1"), summary("archived-2")], + new Set(["visible"]) + ); + expect([...impact.keys()]).toEqual(["visible"]); + }); + + it("derives committed files from the committed rate", () => { + const impact = impactFromSummaries( + [summary("visible", { filesChanged: 10, committedRatePercent: 45 })], + new Set(["visible"]) + ); + expect(impact.get("visible")).toMatchObject({ + filesChanged: 10, + linesAdded: 40, + linesRemoved: 10, + relatedCommits: 2, + committedFiles: 5, + committedRatePercent: 45, + }); + }); + + it("returns an empty map when nothing is visible", () => { + expect(impactFromSummaries([summary("archived")], new Set()).size).toBe(0); + }); +}); + +describe("sessionImpactRosterKey", () => { + it("is stable across reordering so a re-render does not refetch", () => { + expect(sessionImpactRosterKey([session("b"), session("a")])).toBe( + sessionImpactRosterKey([session("a"), session("b")]) + ); + }); + + it("changes when membership swaps at the same roster size", () => { + expect(sessionImpactRosterKey([session("a"), session("b")])).not.toBe( + sessionImpactRosterKey([session("a"), session("c")]) + ); + }); + + it("is empty for an empty roster", () => { + expect(sessionImpactRosterKey([])).toBe(""); + }); +}); diff --git a/src/features/TaskKanban/hooks/useSessionImpact.ts b/src/features/TaskKanban/hooks/useSessionImpact.ts index a0e038ffbf..103a3332e0 100644 --- a/src/features/TaskKanban/hooks/useSessionImpact.ts +++ b/src/features/TaskKanban/hooks/useSessionImpact.ts @@ -48,11 +48,19 @@ function impactFromSession(session: Session): SessionImpactStats | undefined { }; } -function impactFromSummaries( - summaries: readonly CoreSessionSummary[] +/** + * Project the orgtrack summary payload down to the sessions the board can + * actually render. The command answers for every session ever recorded, so + * retaining its full response in React state grows with the database rather + * than with the visible roster. + */ +export function impactFromSummaries( + summaries: readonly CoreSessionSummary[], + retainedSessionIds: ReadonlySet ): Map { const impactBySessionId = new Map(); for (const summary of summaries) { + if (!retainedSessionIds.has(summary.sessionId)) continue; impactBySessionId.set(summary.sessionId, { filesChanged: summary.filesChanged, linesAdded: summary.linesAdded, @@ -71,6 +79,17 @@ export interface SessionImpactState { impactBySessionId: Map; } +/** `\u0000` cannot appear in a session id, so it is a safe id-list separator. */ +const SESSION_ID_SEPARATOR = "\u0000"; +const EMPTY_IMPACT: ReadonlyMap = new Map(); + +export function sessionImpactRosterKey(sessions: readonly Session[]): string { + return sessions + .map((session) => session.session_id) + .sort() + .join(SESSION_ID_SEPARATOR); +} + /** * Loads already-parsed session impact stats for the Kanban board. This is a * read-only view: it surfaces source-owned metadata (`impactFromSession`) and @@ -81,24 +100,31 @@ export interface SessionImpactState { export function useSessionImpact( sessions: readonly Session[] ): SessionImpactState { - const [summaries, setSummaries] = useState([]); + const [summaryImpact, setSummaryImpact] = + useState>(EMPTY_IMPACT); + // Identity-stable across roster re-renders that do not change membership, + // and it also carries the id set the response must be projected onto — so + // the effect needs no extra ref to read the current roster. + const rosterKey = useMemo(() => sessionImpactRosterKey(sessions), [sessions]); useEffect(() => { - let cancelled = false; + const retainedSessionIds = new Set( + rosterKey ? rosterKey.split(SESSION_ID_SEPARATOR) : [] + ); + // An empty roster needs no request; the read-time projection below drops + // whatever the previous roster left behind. + if (retainedSessionIds.size === 0) return; + let cancelled = false; void (async () => { - if (sessions.length === 0) { - if (!cancelled) { - setSummaries([]); - } - return; - } - try { const nextSummaries = await getOrgtrackSessionSummaries(); - if (!cancelled) { - setSummaries(nextSummaries); - } + if (cancelled) return; + // Only the projection is retained; the full response is released as + // soon as this callback returns. + setSummaryImpact( + impactFromSummaries(nextSummaries, retainedSessionIds) + ); } catch (err) { logger.warn("failed to load orgtrack core summaries", { err }); } @@ -107,18 +133,20 @@ export function useSessionImpact( return () => { cancelled = true; }; - }, [sessions.length]); + }, [rosterKey]); const impactBySessionId = useMemo(() => { - const nextImpact = impactFromSummaries(summaries); + const nextImpact = new Map(); for (const session of sessions) { - const sourceImpact = impactFromSession(session); - if (sourceImpact) { - nextImpact.set(session.session_id, sourceImpact); + // Source-owned stats win over the materialized orgtrack summary. + const impact = + impactFromSession(session) ?? summaryImpact.get(session.session_id); + if (impact) { + nextImpact.set(session.session_id, impact); } } return nextImpact; - }, [sessions, summaries]); + }, [sessions, summaryImpact]); return useMemo(() => ({ impactBySessionId }), [impactBySessionId]); } diff --git a/src/hooks/benchmark/__tests__/TEST_CASES.md b/src/hooks/benchmark/__tests__/TEST_CASES.md new file mode 100644 index 0000000000..ec027ba2de --- /dev/null +++ b/src/hooks/benchmark/__tests__/TEST_CASES.md @@ -0,0 +1,19 @@ +# Benchmark async coordination test cases + +## Task discovery + +- Multiple mounted benchmark consumers requesting the same kind, source path, + and limit share one backend request. +- A failed shared request releases its entry so a later retry can run. +- A changed kind or source path is a distinct scope and cannot reuse the old + result. + +## Status polling + +- Agent-batch and benchmark-run status requests share one in-flight request per + identifier. +- A completed status response is reused only inside the current two-second poll + period. +- Polling never overlaps a still-running request. +- Hidden pages keep no polling timer; becoming visible runs one catch-up pass. +- Cleanup during an active request prevents any subsequent timer. diff --git a/src/hooks/benchmark/__tests__/benchmarkRequestCoordinator.test.ts b/src/hooks/benchmark/__tests__/benchmarkRequestCoordinator.test.ts new file mode 100644 index 0000000000..faeb872c84 --- /dev/null +++ b/src/hooks/benchmark/__tests__/benchmarkRequestCoordinator.test.ts @@ -0,0 +1,111 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { benchmarkApi } from "@src/api/tauri/benchmark"; + +import { + __TESTS_ONLY, + getBenchmarkAgentBatchStatusShared, + listBenchmarkTasksShared, + setBenchmarkAgentBatchStatusShared, +} from "../benchmarkRequestCoordinator"; + +vi.mock("@src/api/tauri/benchmark", async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + benchmarkApi: { + ...original.benchmarkApi, + getAgentBatchStatus: vi.fn(), + listTasks: vi.fn(), + }, + }; +}); + +describe("benchmark request coordinator", () => { + beforeEach(() => { + __TESTS_ONLY.reset(); + vi.mocked(benchmarkApi.getAgentBatchStatus).mockReset(); + vi.mocked(benchmarkApi.listTasks).mockReset(); + }); + + it("shares task discovery across hook instances", async () => { + vi.mocked(benchmarkApi.listTasks).mockResolvedValue([]); + const request = { + kind: "swe_bench_pro" as const, + sourcePath: "/bench", + limit: 250, + }; + + await Promise.all([ + listBenchmarkTasksShared(request), + listBenchmarkTasksShared(request), + ]); + + expect(benchmarkApi.listTasks).toHaveBeenCalledTimes(1); + }); + + it("shares an active status request and reuses it within one poll period", async () => { + const status = { + batchId: "batch-1", + status: "running", + }; + vi.mocked(benchmarkApi.getAgentBatchStatus).mockResolvedValue( + status as Awaited> + ); + + await Promise.all([ + getBenchmarkAgentBatchStatusShared("batch-1"), + getBenchmarkAgentBatchStatusShared("batch-1"), + ]); + await getBenchmarkAgentBatchStatusShared("batch-1"); + + expect(benchmarkApi.getAgentBatchStatus).toHaveBeenCalledTimes(1); + }); + + it("releases a failed request for retry", async () => { + vi.mocked(benchmarkApi.getAgentBatchStatus) + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce({ + batchId: "batch-1", + status: "running", + } as Awaited>); + + await expect(getBenchmarkAgentBatchStatusShared("batch-1")).rejects.toThrow( + "offline" + ); + await expect( + getBenchmarkAgentBatchStatusShared("batch-1") + ).resolves.toBeTruthy(); + + expect(benchmarkApi.getAgentBatchStatus).toHaveBeenCalledTimes(2); + }); + + it("does not expose an older poll response after a mutation seeds status", async () => { + type AgentStatus = Awaited< + ReturnType + >; + let release!: (status: AgentStatus) => void; + vi.mocked(benchmarkApi.getAgentBatchStatus).mockImplementation( + () => + new Promise((resolve) => { + release = resolve; + }) + ); + const running = { batchId: "batch-1", status: "running" } as AgentStatus; + const cancelled = { + batchId: "batch-1", + status: "cancelled", + } as AgentStatus; + + const poll = getBenchmarkAgentBatchStatusShared("batch-1"); + setBenchmarkAgentBatchStatusShared(cancelled); + release(running); + + await expect(poll).resolves.toBe(cancelled); + await expect(getBenchmarkAgentBatchStatusShared("batch-1")).resolves.toBe( + cancelled + ); + expect(benchmarkApi.getAgentBatchStatus).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/hooks/benchmark/benchmarkRequestCoordinator.ts b/src/hooks/benchmark/benchmarkRequestCoordinator.ts new file mode 100644 index 0000000000..77e9f5fd27 --- /dev/null +++ b/src/hooks/benchmark/benchmarkRequestCoordinator.ts @@ -0,0 +1,185 @@ +import { + type BenchmarkAgentBatchStatus, + type BenchmarkKind, + type BenchmarkRunStatus, + type BenchmarkTaskDetail, + type BenchmarkTaskIndexRow, + benchmarkApi, +} from "@src/api/tauri/benchmark"; + +const STATUS_CACHE_MS = 1_900; +const MAX_STATUS_ENTRIES = 32; + +interface SharedEntry { + fetchedAt?: number; + generation?: number; + inFlight?: Promise; + value?: T; +} + +const taskRequests = new Map>(); +const taskDetailRequests = new Map>(); +const agentBatchHistoryRequests = new Map< + string, + SharedEntry +>(); +const agentBatchStatusRequests = new Map< + string, + SharedEntry +>(); +const runStatusRequests = new Map>(); + +function prune(entries: Map>): void { + if (entries.size <= MAX_STATUS_ENTRIES) return; + const removable = [...entries.entries()] + .filter(([, entry]) => !entry.inFlight) + .sort( + ([, left], [, right]) => (left.fetchedAt ?? 0) - (right.fetchedAt ?? 0) + ); + for (const [key] of removable) { + if (entries.size <= MAX_STATUS_ENTRIES) break; + entries.delete(key); + } +} + +function sharedRequest( + entries: Map>, + key: string, + loader: () => Promise, + options?: { force?: boolean; maxAgeMs?: number } +): Promise { + const entry = entries.get(key) ?? {}; + if (entry.inFlight) return entry.inFlight; + if ( + !options?.force && + entry.value !== undefined && + entry.fetchedAt !== undefined && + Date.now() - entry.fetchedAt < (options?.maxAgeMs ?? 0) + ) { + return Promise.resolve(entry.value); + } + + const requestGeneration = entry.generation ?? 0; + const request = loader().then((value) => { + if ( + (entry.generation ?? 0) !== requestGeneration && + entry.value !== undefined + ) { + return entry.value; + } + return value; + }); + entry.inFlight = request; + entries.set(key, entry); + void request.then( + (value) => { + if (entry.inFlight !== request) return; + entry.value = value; + entry.fetchedAt = Date.now(); + entry.inFlight = undefined; + prune(entries); + }, + () => { + if (entry.inFlight === request) { + entry.inFlight = undefined; + if (entry.value === undefined) entries.delete(key); + } + } + ); + return request; +} + +function seedSharedEntry( + entries: Map>, + key: string, + value: T +): void { + const entry = entries.get(key) ?? {}; + entry.generation = (entry.generation ?? 0) + 1; + entry.inFlight = undefined; + entry.value = value; + entry.fetchedAt = Date.now(); + entries.set(key, entry); + prune(entries); +} + +export function listBenchmarkTasksShared(request: { + kind: BenchmarkKind; + limit: number; + sourcePath: string; +}): Promise { + const key = JSON.stringify([request.kind, request.sourcePath, request.limit]); + return sharedRequest(taskRequests, key, () => + benchmarkApi.listTasks(request) + ); +} + +export function getBenchmarkTaskShared(request: { + kind: BenchmarkKind; + sourcePath: string; + taskId: string; +}): Promise { + const key = JSON.stringify([ + request.kind, + request.sourcePath, + request.taskId, + ]); + return sharedRequest(taskDetailRequests, key, () => + benchmarkApi.getTask(request) + ); +} + +export function listBenchmarkAgentBatchHistoriesShared( + limit: number +): Promise { + return sharedRequest( + agentBatchHistoryRequests, + String(limit), + () => benchmarkApi.listAgentBatchHistories({ limit }), + { maxAgeMs: STATUS_CACHE_MS } + ); +} + +export function getBenchmarkAgentBatchStatusShared( + batchId: string, + options?: { force?: boolean } +): Promise { + return sharedRequest( + agentBatchStatusRequests, + batchId, + () => benchmarkApi.getAgentBatchStatus({ batchId }), + { force: options?.force, maxAgeMs: STATUS_CACHE_MS } + ); +} + +export function setBenchmarkAgentBatchStatusShared( + status: BenchmarkAgentBatchStatus +): void { + seedSharedEntry(agentBatchStatusRequests, status.batchId, status); +} + +export function getBenchmarkRunStatusShared( + runId: string, + options?: { force?: boolean } +): Promise { + return sharedRequest( + runStatusRequests, + runId, + () => benchmarkApi.getRunStatus({ runId }), + { force: options?.force, maxAgeMs: STATUS_CACHE_MS } + ); +} + +export function setBenchmarkRunStatusShared(status: BenchmarkRunStatus): void { + seedSharedEntry(runStatusRequests, status.runId, status); +} + +export const __TESTS_ONLY = { + reset() { + taskRequests.clear(); + taskDetailRequests.clear(); + agentBatchHistoryRequests.clear(); + agentBatchStatusRequests.clear(); + runStatusRequests.clear(); + }, +}; diff --git a/src/hooks/benchmark/useBenchmarkAgentBatchRun.ts b/src/hooks/benchmark/useBenchmarkAgentBatchRun.ts index cd4ebba3ba..17d1494d75 100644 --- a/src/hooks/benchmark/useBenchmarkAgentBatchRun.ts +++ b/src/hooks/benchmark/useBenchmarkAgentBatchRun.ts @@ -37,6 +37,13 @@ import { chatPanelContentModeAtom, chatPanelMaximizedAtom, } from "@src/store/ui/chatPanelAtom"; +import { startVisibilityAwarePoll } from "@src/util/core/visibilityAwarePoll"; + +import { + getBenchmarkAgentBatchStatusShared, + listBenchmarkAgentBatchHistoriesShared, + setBenchmarkAgentBatchStatusShared, +} from "./benchmarkRequestCoordinator"; const AGENT_BATCH_STATUS_POLL_INTERVAL_MS = 2_000; @@ -139,9 +146,10 @@ export function useBenchmarkAgentBatchRun() { if (!batchStatus?.batchId) { return null; } - const nextStatus = await benchmarkApi.getAgentBatchStatus({ - batchId: batchStatus.batchId, - }); + const nextStatus = await getBenchmarkAgentBatchStatusShared( + batchStatus.batchId, + { force: true } + ); setBatchStatus(nextStatus); return nextStatus; }, [batchStatus?.batchId, setBatchStatus]); @@ -177,6 +185,7 @@ export function useBenchmarkAgentBatchRun() { launch, concurrency, }); + setBenchmarkAgentBatchStatusShared(status); setBatchStatus(status); setActiveBatchId(status.batchId); setActiveBatchTaskId(null); @@ -223,6 +232,7 @@ export function useBenchmarkAgentBatchRun() { const status = await benchmarkApi.cancelAgentBatch({ batchId: batchStatus.batchId, }); + setBenchmarkAgentBatchStatusShared(status); setBatchStatus(status); return status; } catch (error) { @@ -239,8 +249,7 @@ export function useBenchmarkAgentBatchRun() { return undefined; } let cancelled = false; - benchmarkApi - .listAgentBatchHistories({ limit: 1 }) + listBenchmarkAgentBatchHistoriesShared(1) .then((histories) => { if (cancelled || histories.length === 0) return; const [latestHistory] = histories; @@ -268,26 +277,27 @@ export function useBenchmarkAgentBatchRun() { } let cancelled = false; - const intervalId = window.setInterval(() => { - benchmarkApi - .getAgentBatchStatus({ batchId: batchStatus.batchId }) - .then((status) => { - if (!cancelled) { - setBatchStatus(status); - } - }) - .catch((error) => { - if (!cancelled) { - const message = - error instanceof Error ? error.message : String(error); - setBatchError(message); - } - }); - }, AGENT_BATCH_STATUS_POLL_INTERVAL_MS); + const batchId = batchStatus.batchId; + const poll = startVisibilityAwarePoll({ + intervalMs: AGENT_BATCH_STATUS_POLL_INTERVAL_MS, + task: async () => { + const status = await getBenchmarkAgentBatchStatusShared(batchId); + if (!cancelled) { + setBatchStatus(status); + } + }, + onError: (error) => { + if (!cancelled) { + const message = + error instanceof Error ? error.message : String(error); + setBatchError(message); + } + }, + }); return () => { cancelled = true; - window.clearInterval(intervalId); + poll.stop(); }; }, [ batchStatus?.batchId, diff --git a/src/hooks/benchmark/useBenchmarkRun.ts b/src/hooks/benchmark/useBenchmarkRun.ts index 94d44fc975..6eb75c20bd 100644 --- a/src/hooks/benchmark/useBenchmarkRun.ts +++ b/src/hooks/benchmark/useBenchmarkRun.ts @@ -19,6 +19,12 @@ import { benchmarkSourcePathAtom, benchmarkTargetRepoPathAtom, } from "@src/store/benchmark"; +import { startVisibilityAwarePoll } from "@src/util/core/visibilityAwarePoll"; + +import { + getBenchmarkRunStatusShared, + setBenchmarkRunStatusShared, +} from "./benchmarkRequestCoordinator"; const RUN_STATUS_POLL_INTERVAL_MS = 2_000; @@ -124,6 +130,7 @@ export function useBenchmarkRun() { ? targetRepoPath : undefined, }); + setBenchmarkRunStatusShared(status); setRunStatus(status); return status; } catch (error) { @@ -151,6 +158,7 @@ export function useBenchmarkRun() { setRunError(null); try { const status = await benchmarkApi.cancelRun({ runId: runStatus.runId }); + setBenchmarkRunStatusShared(status); setRunStatus(status); } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -169,24 +177,25 @@ export function useBenchmarkRun() { } let cancelled = false; - const intervalId = window.setInterval(() => { - benchmarkApi - .getRunStatus({ runId: runStatus.runId }) - .then((status) => { - if (!cancelled) { - setRunStatus(status); - } - }) - .catch((error) => { - if (!cancelled) { - setRunError(error instanceof Error ? error.message : String(error)); - } - }); - }, RUN_STATUS_POLL_INTERVAL_MS); + const runId = runStatus.runId; + const poll = startVisibilityAwarePoll({ + intervalMs: RUN_STATUS_POLL_INTERVAL_MS, + task: async () => { + const status = await getBenchmarkRunStatusShared(runId); + if (!cancelled) { + setRunStatus(status); + } + }, + onError: (error) => { + if (!cancelled) { + setRunError(error instanceof Error ? error.message : String(error)); + } + }, + }); return () => { cancelled = true; - window.clearInterval(intervalId); + poll.stop(); }; }, [runStatus?.runId, runStatus?.status, setRunError, setRunStatus]); diff --git a/src/hooks/benchmark/useBenchmarkTasks.ts b/src/hooks/benchmark/useBenchmarkTasks.ts index 4d4c183af0..af3615f7b3 100644 --- a/src/hooks/benchmark/useBenchmarkTasks.ts +++ b/src/hooks/benchmark/useBenchmarkTasks.ts @@ -1,7 +1,6 @@ import { useAtom, useSetAtom } from "jotai"; -import { useCallback, useEffect } from "react"; +import { useCallback, useEffect, useMemo } from "react"; -import { benchmarkApi } from "@src/api/tauri/benchmark"; import { BENCHMARK_TASK_LIST_LIMIT, benchmarkErrorAtom, @@ -13,6 +12,12 @@ import { benchmarkTasksAtom, benchmarkTasksLoadingAtom, } from "@src/store/benchmark"; +import { LatestScopedTask } from "@src/util/core/latestScopedTask"; + +import { + getBenchmarkTaskShared, + listBenchmarkTasksShared, +} from "./benchmarkRequestCoordinator"; interface UseBenchmarkTasksOptions { loadDetail?: boolean; @@ -38,75 +43,32 @@ export function useBenchmarkTasks({ ); const [error, setError] = useAtom(benchmarkErrorAtom); const setSelectedTaskAtom = useSetAtom(benchmarkSelectedTaskAtom); + const taskListCoordinator = useMemo(() => new LatestScopedTask(), []); + const taskDetailCoordinator = useMemo(() => new LatestScopedTask(), []); const loadTasks = useCallback(async () => { const trimmedSourcePath = sourcePath.trim(); if (!trimmedSourcePath) { + taskListCoordinator.supersede(); setError(null); setTasks([]); setSelectedTaskId(null); setSelectedTaskAtom(null); - return; - } - - setIsLoadingTasks(true); - setError(null); - try { - const rows = await benchmarkApi.listTasks({ - kind, - sourcePath: trimmedSourcePath, - limit: BENCHMARK_TASK_LIST_LIMIT, - }); - setTasks(rows); - setSelectedTaskId((currentTaskId) => { - if (rows.some((row) => row.taskId === currentTaskId)) { - return currentTaskId; - } - return rows[0]?.taskId ?? null; - }); - } catch (loadError) { - setError( - loadError instanceof Error ? loadError.message : String(loadError) - ); - setTasks([]); - setSelectedTaskId(null); - setSelectedTaskAtom(null); - } finally { setIsLoadingTasks(false); - } - }, [ - kind, - setError, - setIsLoadingTasks, - setSelectedTaskAtom, - setSelectedTaskId, - setTasks, - sourcePath, - ]); - - useEffect(() => { - if (!loadOnMount) return; - - const trimmedSourcePath = sourcePath.trim(); - if (!trimmedSourcePath) { - setError(null); - setTasks([]); - setSelectedTaskId(null); - setSelectedTaskAtom(null); return; } - let cancelled = false; - async function loadInitialTasks() { + const scopeKey = JSON.stringify([kind, trimmedSourcePath]); + await taskListCoordinator.run(scopeKey, async (context) => { setIsLoadingTasks(true); setError(null); try { - const rows = await benchmarkApi.listTasks({ + const rows = await listBenchmarkTasksShared({ kind, sourcePath: trimmedSourcePath, limit: BENCHMARK_TASK_LIST_LIMIT, }); - if (cancelled) return; + if (!context.isCurrent()) return; setTasks(rows); setSelectedTaskId((currentTaskId) => { if (rows.some((row) => row.taskId === currentTaskId)) { @@ -115,7 +77,7 @@ export function useBenchmarkTasks({ return rows[0]?.taskId ?? null; }); } catch (loadError) { - if (cancelled) return; + if (!context.isCurrent()) return; setError( loadError instanceof Error ? loadError.message : String(loadError) ); @@ -123,67 +85,69 @@ export function useBenchmarkTasks({ setSelectedTaskId(null); setSelectedTaskAtom(null); } finally { - if (!cancelled) { + if (context.isCurrent()) { setIsLoadingTasks(false); } } - } - - loadInitialTasks(); - return () => { - cancelled = true; - }; + }); }, [ kind, - loadOnMount, setError, setIsLoadingTasks, setSelectedTaskAtom, setSelectedTaskId, setTasks, sourcePath, + taskListCoordinator, ]); + useEffect(() => { + if (!loadOnMount) return; + void loadTasks(); + return () => { + taskListCoordinator.supersede(); + }; + }, [loadOnMount, loadTasks, taskListCoordinator]); + useEffect(() => { if (!loadDetail) return; if (!selectedTaskId) { + taskDetailCoordinator.supersede(); setSelectedTask(null); + setIsLoadingDetail(false); return; } - let cancelled = false; const taskId = selectedTaskId; - - async function loadTaskDetail() { + const scopeKey = JSON.stringify([kind, sourcePath, taskId]); + void taskDetailCoordinator.run(scopeKey, async (context) => { setIsLoadingDetail(true); setError(null); try { - const detail = await benchmarkApi.getTask({ + const detail = await getBenchmarkTaskShared({ kind, sourcePath, taskId, }); - if (!cancelled) { + if (context.isCurrent()) { setSelectedTask(detail); } } catch (loadError) { - if (!cancelled) { + if (context.isCurrent()) { setError( loadError instanceof Error ? loadError.message : String(loadError) ); setSelectedTask(null); } } finally { - if (!cancelled) { + if (context.isCurrent()) { setIsLoadingDetail(false); } } - } - - loadTaskDetail(); + }); return () => { - cancelled = true; + taskDetailCoordinator.supersede(); }; }, [ kind, @@ -193,6 +157,7 @@ export function useBenchmarkTasks({ setIsLoadingDetail, setSelectedTask, sourcePath, + taskDetailCoordinator, ]); return { diff --git a/src/hooks/git/useRepoSelection/useRepoLoader.ts b/src/hooks/git/useRepoSelection/useRepoLoader.ts index 1aa1e69efb..6dce4b8bcf 100644 --- a/src/hooks/git/useRepoSelection/useRepoLoader.ts +++ b/src/hooks/git/useRepoSelection/useRepoLoader.ts @@ -94,6 +94,7 @@ export function useRepoLoader(): UseRepoLoaderReturn { const isHotReloadRef = useRef(false); const selectedRepoIdRef = useRef(selectedRepoId); const loadGenerationRef = useRef(0); + const forceRefreshRequestedRef = useRef(false); // === HOT RELOAD FIX === if (repos.length > 0 && !loadedReposRef.current && !isHotReloadRef.current) { @@ -124,11 +125,13 @@ export function useRepoLoader(): UseRepoLoaderReturn { } const loadRepos = useCallback(async () => { - if (globalLoadInProgress) { + const forceRefresh = forceRefreshRequestedRef.current; + forceRefreshRequestedRef.current = false; + if (globalLoadInProgress && !forceRefresh) { return; } - if (globalReposLoaded && loadedReposRef.current) { + if (!forceRefresh && globalReposLoaded && loadedReposRef.current) { return; } @@ -139,7 +142,7 @@ export function useRepoLoader(): UseRepoLoaderReturn { let loadSucceeded = false; try { - const response = await getRepos(); + const response = await getRepos({ forceRefresh }); // Discard stale response when forceRefreshRepos() started a newer call. // The newer call already owns globalLoadInProgress and loadingReposRef, @@ -230,7 +233,7 @@ export function useRepoLoader(): UseRepoLoaderReturn { const forceRefreshRepos = useCallback(async () => { loadedReposRef.current = false; setGlobalReposLoaded(false); - setGlobalLoadInProgress(false); + forceRefreshRequestedRef.current = true; await loadRepos(); }, [loadRepos]); diff --git a/src/hooks/platform/useInlineWebview/__tests__/useInlineWebviewNativeVisibility.test.ts b/src/hooks/platform/useInlineWebview/__tests__/useInlineWebviewNativeVisibility.test.ts new file mode 100644 index 0000000000..df6ed6446b --- /dev/null +++ b/src/hooks/platform/useInlineWebview/__tests__/useInlineWebviewNativeVisibility.test.ts @@ -0,0 +1,157 @@ +// @vitest-environment jsdom +import { act, createElement } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { useInlineWebviewNativeVisibility } from "../useInlineWebviewNativeVisibility"; + +const invokeMock = vi.fn(); + +vi.mock("@tauri-apps/api/core", () => ({ + invoke: (...args: unknown[]) => invokeMock(...args), +})); + +const reactActEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; +}; +const labelRef = { current: "browser-session-test" }; + +function deferred(): { + promise: Promise; + resolve: () => void; +} { + let resolve!: () => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +function VisibilityHarness({ + isVisible, + updatePosition, +}: { + isVisible: boolean; + updatePosition: (options?: { + force?: boolean; + show?: boolean; + }) => Promise; +}) { + useInlineWebviewNativeVisibility({ + isWebviewCreated: true, + isVisible, + isWebviewAvailable: true, + labelRef, + updatePosition, + log: vi.fn(), + }); + return null; +} + +describe("useInlineWebviewNativeVisibility", () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + reactActEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + invokeMock.mockReset(); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + Reflect.deleteProperty(reactActEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + it("serializes native transitions and applies the latest visibility intent", async () => { + const hiddenTransition = deferred(); + const updatePosition = vi.fn().mockResolvedValue(undefined); + invokeMock.mockReturnValueOnce(hiddenTransition.promise); + + await act(async () => { + root.render( + createElement(VisibilityHarness, { + isVisible: false, + updatePosition, + }) + ); + await Promise.resolve(); + }); + + expect(invokeMock).toHaveBeenCalledWith("update_inline_webview_position", { + label: "browser-session-test", + x: -10000, + y: -10000, + width: 1, + height: 1, + }); + + await act(async () => { + root.render( + createElement(VisibilityHarness, { + isVisible: true, + updatePosition, + }) + ); + await Promise.resolve(); + }); + expect(updatePosition).not.toHaveBeenCalled(); + + await act(async () => { + hiddenTransition.resolve(); + await hiddenTransition.promise; + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(updatePosition).toHaveBeenCalledTimes(1); + expect(updatePosition).toHaveBeenCalledWith({ force: true, show: true }); + }); + + it("skips a queued show transition that a newer hide supersedes", async () => { + const firstHide = deferred(); + const updatePosition = vi.fn().mockResolvedValue(undefined); + invokeMock + .mockReturnValueOnce(firstHide.promise) + .mockResolvedValueOnce(undefined); + + await act(async () => { + root.render( + createElement(VisibilityHarness, { + isVisible: false, + updatePosition, + }) + ); + await Promise.resolve(); + }); + + act(() => { + root.render( + createElement(VisibilityHarness, { + isVisible: true, + updatePosition, + }) + ); + root.render( + createElement(VisibilityHarness, { + isVisible: false, + updatePosition, + }) + ); + }); + + await act(async () => { + firstHide.resolve(); + await firstHide.promise; + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(updatePosition).not.toHaveBeenCalled(); + expect(invokeMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/hooks/platform/useInlineWebview/__tests__/useWebviewLayout.test.ts b/src/hooks/platform/useInlineWebview/__tests__/useWebviewLayout.test.ts new file mode 100644 index 0000000000..b96ea08e3a --- /dev/null +++ b/src/hooks/platform/useInlineWebview/__tests__/useWebviewLayout.test.ts @@ -0,0 +1,163 @@ +// @vitest-environment jsdom +import { act, createElement, useEffect } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { UseWebviewLayoutReturn } from "../useWebviewLayout"; +import { useWebviewLayout } from "../useWebviewLayout"; +import { WEBVIEW_LAYOUT_CHANGED_EVENT } from "../webviewLayoutEvents"; + +const invokeMock = vi.fn().mockResolvedValue(undefined); + +vi.mock("@tauri-apps/api/core", () => ({ + invoke: (...args: unknown[]) => invokeMock(...args), +})); + +const reactActEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; +}; +const labelRef = { current: "browser-session-layout-test" }; +const layoutContainerRef = { current: null as HTMLDivElement | null }; + +class ResizeObserverMock { + static instances: ResizeObserverMock[] = []; + readonly disconnect = vi.fn(); + readonly observe = vi.fn(); + + constructor(readonly callback: ResizeObserverCallback) { + ResizeObserverMock.instances.push(this); + } + + unobserve(): void {} +} + +let latestLayout: UseWebviewLayoutReturn | null = null; + +function LayoutHarness({ isVisible }: { isVisible: boolean }) { + const layout = useWebviewLayout({ + containerRef: layoutContainerRef, + isWebviewCreated: true, + isWebviewAvailable: true, + isVisible, + labelRef, + log: vi.fn(), + }); + useEffect(() => { + latestLayout = layout; + return () => { + latestLayout = null; + }; + }, [layout]); + return null; +} + +describe("useWebviewLayout visibility lifecycle", () => { + let container: HTMLDivElement; + let root: Root; + let rectSpy: ReturnType; + + beforeEach(() => { + reactActEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + invokeMock.mockClear(); + ResizeObserverMock.instances = []; + globalThis.ResizeObserver = + ResizeObserverMock as unknown as typeof ResizeObserver; + layoutContainerRef.current = document.createElement("div"); + rectSpy = vi + .spyOn(HTMLElement.prototype, "getBoundingClientRect") + .mockReturnValue({ + x: 10, + y: 20, + left: 10, + top: 20, + right: 310, + bottom: 220, + width: 300, + height: 200, + toJSON: () => ({}), + }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + rectSpy.mockRestore(); + latestLayout = null; + layoutContainerRef.current = null; + Reflect.deleteProperty(reactActEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + it("disconnects observers and ignores layout work while hidden", async () => { + act(() => { + root.render(createElement(LayoutHarness, { isVisible: true })); + }); + expect(ResizeObserverMock.instances).toHaveLength(1); + + await act(async () => { + await latestLayout!.updatePosition({ force: true }); + }); + expect(invokeMock).toHaveBeenLastCalledWith( + "update_inline_webview_position", + expect.objectContaining({ + label: "browser-session-layout-test", + x: 10, + y: 20, + width: 300, + height: 200, + }) + ); + + act(() => { + root.render(createElement(LayoutHarness, { isVisible: false })); + }); + expect(ResizeObserverMock.instances[0].disconnect).toHaveBeenCalledTimes(1); + + invokeMock.mockClear(); + await act(async () => { + await latestLayout!.updatePosition({ force: true }); + window.dispatchEvent(new Event(WEBVIEW_LAYOUT_CHANGED_EVENT)); + await Promise.resolve(); + }); + expect(invokeMock).not.toHaveBeenCalled(); + }); + + it("uses the atomic reposition-and-show command when becoming visible", async () => { + act(() => { + root.render(createElement(LayoutHarness, { isVisible: true })); + }); + + await act(async () => { + await latestLayout!.updatePosition({ force: true, show: true }); + }); + + expect(invokeMock).toHaveBeenCalledWith( + "reposition_and_show_webview", + expect.objectContaining({ + label: "browser-session-layout-test", + x: 10, + y: 20, + width: 300, + height: 200, + }) + ); + }); + + it("cleans up every observer across repeated visible/hidden cycles", () => { + for (let index = 0; index < 20; index += 1) { + act(() => { + root.render(createElement(LayoutHarness, { isVisible: true })); + }); + act(() => { + root.render(createElement(LayoutHarness, { isVisible: false })); + }); + } + + expect(ResizeObserverMock.instances).toHaveLength(20); + for (const observer of ResizeObserverMock.instances) { + expect(observer.disconnect).toHaveBeenCalledTimes(1); + } + }); +}); diff --git a/src/hooks/platform/useInlineWebview/useInlineWebview.ts b/src/hooks/platform/useInlineWebview/useInlineWebview.ts index 0ebfea3af7..dba41941ee 100644 --- a/src/hooks/platform/useInlineWebview/useInlineWebview.ts +++ b/src/hooks/platform/useInlineWebview/useInlineWebview.ts @@ -77,6 +77,7 @@ export function useInlineWebview( containerRef, isWebviewCreated, isWebviewAvailable, + isVisible, labelRef, log, }); diff --git a/src/hooks/platform/useInlineWebview/useInlineWebviewNativeVisibility.ts b/src/hooks/platform/useInlineWebview/useInlineWebviewNativeVisibility.ts index e9bb5adfd4..32befd22ca 100644 --- a/src/hooks/platform/useInlineWebview/useInlineWebviewNativeVisibility.ts +++ b/src/hooks/platform/useInlineWebview/useInlineWebviewNativeVisibility.ts @@ -1,12 +1,15 @@ import { invoke } from "@tauri-apps/api/core"; -import { type MutableRefObject, useEffect } from "react"; +import { type MutableRefObject, useEffect, useRef } from "react"; export interface UseInlineWebviewNativeVisibilityParams { isWebviewCreated: boolean; isVisible: boolean; isWebviewAvailable: boolean; labelRef: MutableRefObject; - updatePosition: (options?: { force?: boolean }) => Promise; + updatePosition: (options?: { + force?: boolean; + show?: boolean; + }) => Promise; log: (...args: unknown[]) => void; } @@ -21,22 +24,21 @@ export function useInlineWebviewNativeVisibility( updatePosition, log, } = params; + const transitionGenerationRef = useRef(0); + const transitionQueueRef = useRef>(Promise.resolve()); useEffect(() => { if (!isWebviewCreated || !isWebviewAvailable) return; - let cancelled = false; + const generation = ++transitionGenerationRef.current; const handleVisibility = async () => { + if (generation !== transitionGenerationRef.current) return; + try { if (isVisible) { log("Showing WebView (isVisible=true)"); - await updatePosition({ force: true }); - if (cancelled) return; - await invoke("set_inline_webview_visibility", { - label: labelRef.current, - visible: true, - }); + await updatePosition({ force: true, show: true }); } else { log("Staging WebView offscreen (isVisible=false, but still mounted)"); await invoke("update_inline_webview_position", { @@ -48,16 +50,24 @@ export function useInlineWebviewNativeVisibility( }); } } catch (err) { - if (!cancelled) { + if (generation === transitionGenerationRef.current) { log("Visibility change failed:", err); } } }; - void handleVisibility(); + // Native WKWebView mutations are serialized per React owner. A newer + // visibility intent invalidates queued work before it reaches Tauri, while + // an already-running mutation is allowed to finish before the latest + // transition applies the final state. + transitionQueueRef.current = transitionQueueRef.current + .catch(() => undefined) + .then(handleVisibility); return () => { - cancelled = true; + if (transitionGenerationRef.current === generation) { + transitionGenerationRef.current += 1; + } }; }, [ isWebviewCreated, diff --git a/src/hooks/platform/useInlineWebview/useWebviewLayout.ts b/src/hooks/platform/useInlineWebview/useWebviewLayout.ts index 25f299d88f..7ceee84969 100644 --- a/src/hooks/platform/useInlineWebview/useWebviewLayout.ts +++ b/src/hooks/platform/useInlineWebview/useWebviewLayout.ts @@ -22,20 +22,30 @@ export interface UseWebviewLayoutParams { containerRef: RefObject; isWebviewCreated: boolean; isWebviewAvailable: boolean; + isVisible: boolean; labelRef: MutableRefObject; log: (...args: unknown[]) => void; } export interface UseWebviewLayoutReturn { getContainerRect: () => DOMRect | null; - updatePosition: (options?: { force?: boolean }) => Promise; + updatePosition: (options?: { + force?: boolean; + show?: boolean; + }) => Promise; } export function useWebviewLayout( params: UseWebviewLayoutParams ): UseWebviewLayoutReturn { - const { containerRef, isWebviewCreated, isWebviewAvailable, labelRef, log } = - params; + const { + containerRef, + isWebviewCreated, + isWebviewAvailable, + isVisible, + labelRef, + log, + } = params; const resizeObserverRef = useRef(null); const scrollListenerRef = useRef<(() => void) | null>(null); @@ -52,8 +62,8 @@ export function useWebviewLayout( }, [containerRef]); const updatePosition = useCallback( - async (options?: { force?: boolean }) => { - if (!isWebviewCreated || !containerRef.current) return; + async (options?: { force?: boolean; show?: boolean }) => { + if (!isWebviewCreated || !isVisible || !containerRef.current) return; const rect = getContainerRect(); if (!rect) return; @@ -86,16 +96,21 @@ export function useWebviewLayout( lastResizeRect.current = nativeFrame; try { - await invoke("update_inline_webview_position", { - label: labelRef.current, - ...nativeFrame, - }); + await invoke( + options?.show + ? "reposition_and_show_webview" + : "update_inline_webview_position", + { + label: labelRef.current, + ...nativeFrame, + } + ); log("Position updated:", { rect, nativeFrame }); } catch (err) { log("Failed to update position:", err); } }, - [isWebviewCreated, containerRef, getContainerRect, labelRef, log] + [isWebviewCreated, isVisible, containerRef, getContainerRect, labelRef, log] ); const debouncedUpdatePosition = useDebouncedCallback(() => { @@ -103,7 +118,7 @@ export function useWebviewLayout( }, DEBOUNCE_DELAYS.FRAME); useEffect(() => { - if (!containerRef.current || !isWebviewAvailable) return; + if (!containerRef.current || !isWebviewAvailable || !isVisible) return; resizeObserverRef.current = new ResizeObserver(() => { debouncedUpdatePosition(); @@ -115,10 +130,10 @@ export function useWebviewLayout( resizeObserverRef.current?.disconnect(); debouncedUpdatePosition.cancel(); }; - }, [containerRef, isWebviewAvailable, debouncedUpdatePosition]); + }, [containerRef, isWebviewAvailable, isVisible, debouncedUpdatePosition]); useEffect(() => { - if (!isWebviewCreated || !isWebviewAvailable) return; + if (!isWebviewCreated || !isWebviewAvailable || !isVisible) return; const scaleUpdateTimers = new Set(); @@ -191,6 +206,7 @@ export function useWebviewLayout( containerRef, isWebviewCreated, isWebviewAvailable, + isVisible, debouncedUpdatePosition, updatePosition, ]); diff --git a/src/modules/ProjectManager/Projects/index.tsx b/src/modules/ProjectManager/Projects/index.tsx index 85873ee33d..8f11102f40 100644 --- a/src/modules/ProjectManager/Projects/index.tsx +++ b/src/modules/ProjectManager/Projects/index.tsx @@ -53,6 +53,7 @@ import { Placeholder } from "@src/modules/shared/layouts/blocks"; import { ContentSearchPalette } from "@src/scaffold/GlobalSpotlight/palettes"; import { projectListRefreshAtom } from "@src/store/project/projectAtom"; import type { Project } from "@src/types/core/project"; +import { LatestScopedTask } from "@src/util/core/latestScopedTask"; import { confirmDestructiveAction } from "@src/util/dialogs/confirmDestructiveAction"; import { ProjectRow, ProjectsPageHeader } from "./components"; @@ -149,54 +150,43 @@ const ProjectsPage: React.FC = ({ const [fileProjectsLoading, setFileProjectsLoading] = useState(false); const [fileProjectsLoaded, setFileProjectsLoaded] = useState(false); const fileProjectsLoadedRef = useRef(false); - const loadLifecycleRef = useRef({ mounted: true, generation: 0 }); const [fileError, setFileError] = useState(null); - - useEffect(() => { - const lifecycle = loadLifecycleRef.current; - lifecycle.mounted = true; - return () => { - lifecycle.mounted = false; - lifecycle.generation += 1; - }; - }, []); + const projectLoadCoordinator = useMemo(() => new LatestScopedTask(), []); const loadProjectsForRepo = useCallback(async () => { - const generation = ++loadLifecycleRef.current.generation; - const isCurrent = () => { - const lifecycle = loadLifecycleRef.current; - return lifecycle.mounted && lifecycle.generation === generation; - }; - setFileProjectsLoading(true); - setFileError(null); - try { - const [projectsData, linearProjects] = await Promise.all([ - projectApi.readProjects({ orgId }), - includeExternalSources ? loadWorkspaceLinearProjects() : [], - ]); - if (!isCurrent()) return; - const localProjects = projectsData.map((project) => - projectDataToUI(project, { - labelMap: EMPTY_LABEL_MAP, - memberMap: EMPTY_MEMBER_MAP, - }) - ); - setFileProjects([...localProjects, ...linearProjects]); - fileProjectsLoadedRef.current = true; - setFileProjectsLoaded(true); - } catch (err) { - if (!isCurrent()) return; - log.error("[ProjectsPage] Failed to load projects:", err); - if (!fileProjectsLoadedRef.current) { - setFileProjects([]); + const scopeKey = JSON.stringify([orgId ?? null, includeExternalSources]); + await projectLoadCoordinator.run(scopeKey, async (context) => { + setFileProjectsLoading(true); + setFileError(null); + try { + const [projectsData, linearProjects] = await Promise.all([ + projectApi.readProjects({ orgId }), + includeExternalSources ? loadWorkspaceLinearProjects() : [], + ]); + if (!context.isCurrent()) return; + const localProjects = projectsData.map((project) => + projectDataToUI(project, { + labelMap: EMPTY_LABEL_MAP, + memberMap: EMPTY_MEMBER_MAP, + }) + ); + setFileProjects([...localProjects, ...linearProjects]); + fileProjectsLoadedRef.current = true; + setFileProjectsLoaded(true); + } catch (err) { + if (!context.isCurrent()) return; + log.error("[ProjectsPage] Failed to load projects:", err); + if (!fileProjectsLoadedRef.current) { + setFileProjects([]); + } + setFileError( + err instanceof Error ? err.message : t("projects.loadProjectsFailed") + ); + } finally { + if (context.isCurrent()) setFileProjectsLoading(false); } - setFileError( - err instanceof Error ? err.message : t("projects.loadProjectsFailed") - ); - } finally { - if (isCurrent()) setFileProjectsLoading(false); - } - }, [includeExternalSources, orgId, t]); + }); + }, [includeExternalSources, orgId, projectLoadCoordinator, t]); const loadFileProjects = useCallback(async () => { await loadProjectsForRepo(); @@ -204,7 +194,10 @@ const ProjectsPage: React.FC = ({ useEffect(() => { void loadProjectsForRepo(); - }, [loadProjectsForRepo, refreshSignal]); + return () => { + projectLoadCoordinator.supersede(); + }; + }, [loadProjectsForRepo, projectLoadCoordinator, refreshSignal]); useProjectDataChanged( useCallback(() => { diff --git a/src/modules/ProjectManager/WorkItems/hooks/__tests__/TEST_CASES.md b/src/modules/ProjectManager/WorkItems/hooks/__tests__/TEST_CASES.md new file mode 100644 index 0000000000..c987150c0b --- /dev/null +++ b/src/modules/ProjectManager/WorkItems/hooks/__tests__/TEST_CASES.md @@ -0,0 +1,18 @@ +# Work Items async loading test cases + +## View-data requests + +- The mount effect and a same-scope project-data event share one request. +- Filtered reads with the same project, status, and search query share only + their active IPC request; a later intentional refresh still reaches Rust. +- Changing project, status, or debounced search supersedes the prior scope. +- A late response from the superseded scope cannot replace the visible data, + error, or loading state. +- Failed requests release their scope so the same filters can retry. + +## Workspace aggregates + +- Mount, manual refresh, and a project-data event share equal in-flight work. +- Switching org or external-source mode starts a new generation. +- Linear and local results from an older generation cannot overwrite the new + workspace selection. diff --git a/src/modules/ProjectManager/WorkItems/hooks/useWorkItemsData.ts b/src/modules/ProjectManager/WorkItems/hooks/useWorkItemsData.ts index eaa9b0e081..54b29b7a21 100644 --- a/src/modules/ProjectManager/WorkItems/hooks/useWorkItemsData.ts +++ b/src/modules/ProjectManager/WorkItems/hooks/useWorkItemsData.ts @@ -27,6 +27,7 @@ import { useDebouncedCallback } from "@src/hooks/perf"; import { useProjectDataChanged } from "@src/hooks/project"; import { useCurrentUserMemberIds } from "@src/hooks/project/useCurrentUserMemberId"; import type { WorkItem as WorkItemExtended } from "@src/types/core/workItem"; +import { LatestScopedTask } from "@src/util/core/latestScopedTask"; import { type OnAssignmentChanges, @@ -124,7 +125,7 @@ export function useWorkItemsData({ const [viewData, setViewData] = useState(null); const [viewLoading, setViewLoading] = useState(false); const [viewError, setViewError] = useState(null); - const loadGenerationRef = useRef(0); + const viewLoadCoordinator = useMemo(() => new LatestScopedTask(), []); const purgedProjectSlugRef = useRef(null); // Debounced search query for IPC calls (avoid IPC on every keystroke) @@ -140,60 +141,79 @@ export function useWorkItemsData({ }, [searchQuery, debouncedSetSearchQuery]); const fetchViewData = useCallback(async () => { - if (!isActive) return; + if (!isActive) { + viewLoadCoordinator.supersede(); + setViewLoading(false); + return; + } if (!projectSlug) { + viewLoadCoordinator.supersede(); setViewData(null); + setViewLoading(false); return; } - const loadGeneration = loadGenerationRef.current + 1; - loadGenerationRef.current = loadGeneration; - setViewLoading(true); - setViewError(null); + const normalizedSearch = debouncedSearchQuery.trim(); + const scopeKey = JSON.stringify([ + projectSlug, + statusFilter, + normalizedSearch, + activeView, + ]); + await viewLoadCoordinator.run(scopeKey, async (context) => { + setViewLoading(true); + setViewError(null); - try { - if (purgedProjectSlugRef.current !== projectSlug) { - await projectApi.purgeExpiredDeletedWorkItems(projectSlug); - if (loadGenerationRef.current !== loadGeneration) return; - purgedProjectSlugRef.current = projectSlug; - } - const data = await projectApi.readWorkItemsViewData(projectSlug, { - statusFilter: statusFilter !== "all" ? statusFilter : undefined, - searchQuery: debouncedSearchQuery.trim() || undefined, - view: - activeView === "Kanban" - ? "kanban" - : activeView === "Gantt" - ? "gantt" - : activeView === "Calendar" - ? "calendar" - : "list", - }); - if (loadGenerationRef.current !== loadGeneration) return; - setViewData(data); - } catch (err) { - if (loadGenerationRef.current !== loadGeneration) return; - const message = - err instanceof Error ? err.message : "Failed to load work items"; - logger.error("View data fetch error:", err); - setViewError(message); - } finally { - if (loadGenerationRef.current === loadGeneration) { - setViewLoading(false); + try { + if (purgedProjectSlugRef.current !== projectSlug) { + await projectApi.purgeExpiredDeletedWorkItems(projectSlug); + if (!context.isCurrent()) return; + purgedProjectSlugRef.current = projectSlug; + } + const data = await projectApi.readWorkItemsViewData(projectSlug, { + statusFilter: statusFilter !== "all" ? statusFilter : undefined, + searchQuery: normalizedSearch || undefined, + view: + activeView === "Kanban" + ? "kanban" + : activeView === "Gantt" + ? "gantt" + : activeView === "Calendar" + ? "calendar" + : "list", + }); + if (context.isCurrent()) { + setViewData(data); + } + } catch (err) { + if (!context.isCurrent()) return; + const message = + err instanceof Error ? err.message : "Failed to load work items"; + logger.error("View data fetch error:", err); + setViewError(message); + } finally { + if (context.isCurrent()) { + setViewLoading(false); + } } - } - }, [activeView, debouncedSearchQuery, isActive, projectSlug, statusFilter]); + }); + }, [ + activeView, + debouncedSearchQuery, + isActive, + projectSlug, + statusFilter, + viewLoadCoordinator, + ]); useEffect(() => { if (!isActive) { - loadGenerationRef.current += 1; + viewLoadCoordinator.supersede(); return; } void fetchViewData(); - return () => { - loadGenerationRef.current += 1; - }; - }, [fetchViewData, isActive]); + return () => viewLoadCoordinator.supersede(); + }, [fetchViewData, isActive, viewLoadCoordinator]); // Listen for orgii-data-changed events useProjectDataChanged( diff --git a/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/__tests__/filterAutoLoad.test.ts b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/__tests__/filterAutoLoad.test.ts new file mode 100644 index 0000000000..d07599c2de --- /dev/null +++ b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/__tests__/filterAutoLoad.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from "vitest"; + +import { + MAX_FILTER_AUTO_SCAN_COMMITS, + extendFilterScanBudget, + isFilterScanCapped, + resolveFilterScanBudget, + shouldAutoLoadMoreForFilter, +} from "../filterAutoLoad"; + +function scan( + overrides: Partial[0]> = {} +) { + return { + filterQuery: "fix", + matchCount: 0, + loadedCommitCount: 25, + hasMore: true, + ...overrides, + }; +} + +describe("shouldAutoLoadMoreForFilter", () => { + it("pages the next window while the filter has no match", () => { + expect(shouldAutoLoadMoreForFilter(scan())).toBe(true); + }); + + it("stops as soon as the filter matches something", () => { + expect(shouldAutoLoadMoreForFilter(scan({ matchCount: 1 }))).toBe(false); + }); + + it("stops without a filter query", () => { + expect(shouldAutoLoadMoreForFilter(scan({ filterQuery: "" }))).toBe(false); + }); + + it("stops when history is exhausted", () => { + expect(shouldAutoLoadMoreForFilter(scan({ hasMore: false }))).toBe(false); + }); + + it("stops at the scan bound instead of paging the whole repository", () => { + expect( + shouldAutoLoadMoreForFilter( + scan({ loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS }) + ) + ).toBe(false); + expect( + shouldAutoLoadMoreForFilter( + scan({ loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS - 1 }) + ) + ).toBe(true); + }); + + it("honors an explicitly granted larger bound", () => { + expect( + shouldAutoLoadMoreForFilter( + scan({ + loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS, + maxScannedCommits: MAX_FILTER_AUTO_SCAN_COMMITS * 2, + }) + ) + ).toBe(true); + }); +}); + +describe("isFilterScanCapped", () => { + it("reports the bound only when the chain stopped on it", () => { + expect( + isFilterScanCapped( + scan({ loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS }) + ) + ).toBe(true); + expect(isFilterScanCapped(scan({ loadedCommitCount: 25 }))).toBe(false); + }); + + it("does not report the bound when history simply ran out", () => { + expect( + isFilterScanCapped( + scan({ + loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS, + hasMore: false, + }) + ) + ).toBe(false); + }); + + it("does not report the bound once a match exists", () => { + expect( + isFilterScanCapped( + scan({ + loadedCommitCount: MAX_FILTER_AUTO_SCAN_COMMITS, + matchCount: 3, + }) + ) + ).toBe(false); + }); +}); + +describe("filter scan budget", () => { + it("defaults to the base bound when no budget was granted", () => { + expect(resolveFilterScanBudget(null, "fix")).toBe( + MAX_FILTER_AUTO_SCAN_COMMITS + ); + }); + + it("defaults to the base bound for an unfiltered list", () => { + expect(resolveFilterScanBudget(null, undefined)).toBe( + MAX_FILTER_AUTO_SCAN_COMMITS + ); + expect( + resolveFilterScanBudget(extendFilterScanBudget("", 0), undefined) + ).toBe(MAX_FILTER_AUTO_SCAN_COMMITS); + }); + + it("ignores a budget granted for a different query", () => { + const budget = extendFilterScanBudget("fix", 500); + expect(resolveFilterScanBudget(budget, "fix")).toBe(1_000); + expect(resolveFilterScanBudget(budget, "refactor")).toBe( + MAX_FILTER_AUTO_SCAN_COMMITS + ); + }); + + it("grants one more window from the current position", () => { + expect(extendFilterScanBudget("fix", 500)).toEqual({ + query: "fix", + limit: 500 + MAX_FILTER_AUTO_SCAN_COMMITS, + }); + }); +}); diff --git a/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/filterAutoLoad.ts b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/filterAutoLoad.ts new file mode 100644 index 0000000000..9703064f28 --- /dev/null +++ b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/filterAutoLoad.ts @@ -0,0 +1,76 @@ +/** + * Bounds for the message-filter auto-paging chain in Git History. + * + * The commit filter matches client-side over the pages already loaded, so a + * query with no match on the current page keeps requesting the next page to + * keep older matches discoverable. Without a bound that walks the entire + * repository history into React state — an unbounded, purely incidental + * memory cost for a single keystroke. The chain is therefore capped, and the + * user can spend another window explicitly from the empty state. + */ +export const MAX_FILTER_AUTO_SCAN_COMMITS = 500; + +export interface FilterScanInput { + /** Undefined while the history list is unfiltered. */ + filterQuery: string | undefined; + /** Commits matching the current filter across everything loaded so far. */ + matchCount: number; + /** Commits currently held in component state. */ + loadedCommitCount: number; + hasMore: boolean; + /** Defaults to {@link MAX_FILTER_AUTO_SCAN_COMMITS}. */ + maxScannedCommits?: number; +} + +function isUnmatchedFilterScan(input: FilterScanInput): boolean { + return Boolean(input.filterQuery) && input.matchCount === 0 && input.hasMore; +} + +/** Whether the filter chain may pull one more page automatically. */ +export function shouldAutoLoadMoreForFilter(input: FilterScanInput): boolean { + if (!isUnmatchedFilterScan(input)) return false; + return ( + input.loadedCommitCount < + (input.maxScannedCommits ?? MAX_FILTER_AUTO_SCAN_COMMITS) + ); +} + +/** Whether the chain stopped on its bound rather than on exhausted history. */ +export function isFilterScanCapped(input: FilterScanInput): boolean { + if (!isUnmatchedFilterScan(input)) return false; + return ( + input.loadedCommitCount >= + (input.maxScannedCommits ?? MAX_FILTER_AUTO_SCAN_COMMITS) + ); +} + +/** + * Budget granted for the current query. A budget recorded for a different + * query is ignored, so changing the filter always restarts from the default + * bound without an extra state-resetting effect. + */ +export interface FilterScanBudget { + query: string; + limit: number; +} + +export function resolveFilterScanBudget( + budget: FilterScanBudget | null, + filterQuery: string | undefined +): number { + if (!budget || !filterQuery || budget.query !== filterQuery) { + return MAX_FILTER_AUTO_SCAN_COMMITS; + } + return budget.limit; +} + +/** Grant one more scan window from the currently loaded position. */ +export function extendFilterScanBudget( + filterQuery: string | undefined, + loadedCommitCount: number +): FilterScanBudget { + return { + query: filterQuery ?? "", + limit: loadedCommitCount + MAX_FILTER_AUTO_SCAN_COMMITS, + }; +} diff --git a/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/index.tsx b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/index.tsx index f62baf4fd0..218cf618f7 100644 --- a/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/index.tsx +++ b/src/modules/WorkStation/CodeEditor/Panels/EditorPrimarySidebar/content/GitHistoryContent/index.tsx @@ -45,6 +45,13 @@ import { import { formatRelativeTime } from "@src/util/time/formatRelativeTime"; import GitHistoryContextMenu from "./GitHistoryContextMenu"; +import { + type FilterScanBudget, + extendFilterScanBudget, + isFilterScanCapped, + resolveFilterScanBudget, + shouldAutoLoadMoreForFilter, +} from "./filterAutoLoad"; import { type CommitGraphNode, DOT_RADIUS, @@ -285,6 +292,8 @@ const GitHistoryContentInner: React.FC = ({ () => initialHistory?.hasMore ?? false ); const [error, setError] = useState(null); + const [filterScanBudget, setFilterScanBudget] = + useState(null); const [contextMenuCommit, setContextMenuCommit] = useState(null); const loadGenerationRef = useRef(0); @@ -401,13 +410,28 @@ const GitHistoryContentInner: React.FC = ({ return commits.filter((c) => c.summary.toLowerCase().includes(lower)); }, [commits, filterQuery]); + const filterScan = { + filterQuery, + matchCount: filteredCommits.length, + loadedCommitCount: commits.length, + hasMore, + maxScannedCommits: resolveFilterScanBudget(filterScanBudget, filterQuery), + }; + const filterScanCapped = isFilterScanCapped(filterScan); + const autoLoadMoreForFilter = shouldAutoLoadMoreForFilter(filterScan); + // The old visible sentinel continued paging when the loaded page had no // filter matches. Keep that behavior so a match in older history remains - // discoverable even though there is no virtual row to reach yet. + // discoverable even though there is no virtual row to reach yet — bounded, + // so one keystroke cannot page an entire repository into memory. useEffect(() => { - if (!filterQuery || filteredCommits.length > 0 || !hasMore) return; + if (!autoLoadMoreForFilter) return; void handleLoadMore(); - }, [filterQuery, filteredCommits.length, handleLoadMore, hasMore]); + }, [autoLoadMoreForFilter, handleLoadMore]); + + const handleSearchOlderCommits = useCallback(() => { + setFilterScanBudget(extendFilterScanBudget(filterQuery, commits.length)); + }, [commits.length, filterQuery]); // Compute graph layout — pure function, deterministic output for same input // When a filter is active we skip the graph (flat list only) @@ -517,6 +541,26 @@ const GitHistoryContentInner: React.FC = ({ variant="empty" placement="sidebar" title={t("placeholders.noResults", "No results")} + subtitle={ + filterScanCapped + ? t("git.history.filterScanLimit", { + defaultValue: "Searched the newest {{count}} commits", + count: commits.length, + }) + : undefined + } + action={ + filterScanCapped + ? { + label: t("git.history.searchOlderCommits", { + defaultValue: "Search older commits", + }), + onClick: handleSearchOlderCommits, + disabled: loadingMore, + dataTestId: "git-history-search-older", + } + : undefined + } fillParentHeight /> ) : ( diff --git a/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.test.ts b/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.test.ts index 4df486e609..edf50e8129 100644 --- a/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.test.ts +++ b/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.test.ts @@ -77,4 +77,69 @@ describe("rescanSidebarSessions", () => { forceRefresh: true, }); }); + + it("shares one in-flight rescan between rapid refresh requests", async () => { + let releaseRescan!: () => void; + mocks.externalHistoryRescanSources.mockImplementation( + () => + new Promise((resolve) => { + releaseRescan = resolve; + }) + ); + + const firstRescan = rescanSidebarSessions(); + const secondRescan = rescanSidebarSessions(); + const thirdRescan = rescanSidebarSessions(); + + expect(mocks.externalHistoryRescanSources).toHaveBeenCalledTimes(1); + expect(mocks.loadSessionRoster).not.toHaveBeenCalled(); + + releaseRescan(); + await Promise.all([firstRescan, secondRescan, thirdRescan]); + expect(mocks.loadSessionRoster).toHaveBeenCalledTimes(1); + }); + + it("releases the in-flight guard after a failed rescan", async () => { + mocks.externalHistoryRescanSources + .mockRejectedValueOnce(new Error("scan failed")) + .mockResolvedValueOnce(undefined); + + await expect(rescanSidebarSessions()).rejects.toThrow("scan failed"); + await expect(rescanSidebarSessions()).resolves.toBeUndefined(); + + expect(mocks.externalHistoryRescanSources).toHaveBeenCalledTimes(2); + expect(mocks.loadSessionRoster).toHaveBeenCalledTimes(1); + }); + + it("runs a trailing rescan for a changed scope even if the old scan fails", async () => { + let rejectFirstRescan!: (error: Error) => void; + mocks.externalHistoryRescanSources + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirstRescan = reject; + }) + ) + .mockResolvedValueOnce(undefined); + + const firstRescan = rescanSidebarSessions(); + mocks.store?.set(dataSourceConfigAtom, { + warp: { enabled: false, frequency: "default", lastScannedAt: null }, + }); + const changedScopeRescan = rescanSidebarSessions(); + + expect(mocks.externalHistoryRescanSources).toHaveBeenCalledTimes(1); + rejectFirstRescan(new Error("obsolete scan failed")); + const results = await Promise.allSettled([firstRescan, changedScopeRescan]); + + expect(results.map(({ status }) => status)).toEqual([ + "rejected", + "fulfilled", + ]); + expect(mocks.externalHistoryRescanSources).toHaveBeenCalledTimes(2); + expect(mocks.externalHistoryRescanSources.mock.calls[1][0]).not.toContain( + "warp" + ); + expect(mocks.loadSessionRoster).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.ts b/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.ts index 1ca1f120ef..42345f8b58 100644 --- a/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.ts +++ b/src/scaffold/NavigationSidebar/connectors/WorkstationSidebarConnector/sidebarSessionRefresh.ts @@ -2,6 +2,7 @@ import { useEffect } from "react"; import { IMPORTED_HISTORY_SOURCE_DESCRIPTORS, + type ImportedHistorySourceId, externalHistoryRescanSources, } from "@src/api/tauri/externalHistory"; import { @@ -21,21 +22,41 @@ import { SIDEBAR_SESSION_IDLE_REFRESH_INTERVAL_MS, } from "../sidebarConnectorUtils"; -/** Rescan every enabled external source, then refresh the canonical roster. */ -export async function rescanSidebarSessions(): Promise { - const store = getInstrumentedStore(); +type SessionStore = ReturnType; + +interface SidebarRescanFlight { + scopeKey: string; + promise: Promise; +} + +const rescanInFlightByStore = new WeakMap(); + +function getRescanScope(store: SessionStore): { + scopeKey: string; + sourceIds: ImportedHistorySourceId[]; +} { if (!store.get(externalSessionsEnabledAtom)) { - // External sessions are switched off entirely — nothing to rescan, and - // the sidebar reload below would be a no-op for external categories. - await loadSessionRoster({ forceRefresh: true }); - return; + return { scopeKey: "external-sessions-disabled", sourceIds: [] }; } const config = store.get(dataSourceConfigAtom); const sourceIds = IMPORTED_HISTORY_SOURCE_DESCRIPTORS.filter( ({ sourceId }) => getSourceConfig(config, sourceId).enabled ).map(({ sourceId }) => sourceId); + return { scopeKey: JSON.stringify(sourceIds), sourceIds }; +} - const scanResult = await externalHistoryRescanSources(sourceIds); +async function performSidebarSessionsRescan( + store: SessionStore, + sourceIds: readonly ImportedHistorySourceId[] +): Promise { + if (!store.get(externalSessionsEnabledAtom)) { + // External sessions are switched off entirely — nothing to rescan, and + // the sidebar reload below would be a no-op for external categories. + await loadSessionRoster({ forceRefresh: true }); + return; + } + + const scanResult = await externalHistoryRescanSources([...sourceIds]); // Explicit refresh: reload unconditionally. Even a rescan that wrote // nothing can follow cache writes from other surfaces' syncs (e.g. a // continuation demotion) that the sidebar never rendered. @@ -58,6 +79,32 @@ export async function rescanSidebarSessions(): Promise { }); } +/** Coalesce overlapping refreshes without letting an obsolete scope win. */ +export async function rescanSidebarSessions(): Promise { + const store = getInstrumentedStore(); + const { scopeKey, sourceIds } = getRescanScope(store); + const inFlight = rescanInFlightByStore.get(store); + if (inFlight) { + if (inFlight.scopeKey === scopeKey) return inFlight.promise; + try { + await inFlight.promise; + } catch { + // A failed obsolete scope must not suppress the current source set. + } + return rescanSidebarSessions(); + } + + const pass = performSidebarSessionsRescan(store, sourceIds); + rescanInFlightByStore.set(store, { scopeKey, promise: pass }); + try { + await pass; + } finally { + if (rescanInFlightByStore.get(store)?.promise === pass) { + rescanInFlightByStore.delete(store); + } + } +} + export function useSidebarSessionRefreshEffects(): void { useEffect(() => { void loadSessionRoster(); diff --git a/src/store/session/sessionAtom/__tests__/sidebarLoaders.test.ts b/src/store/session/sessionAtom/__tests__/sidebarLoaders.test.ts index d46fbd330a..498aaf17da 100644 --- a/src/store/session/sessionAtom/__tests__/sidebarLoaders.test.ts +++ b/src/store/session/sessionAtom/__tests__/sidebarLoaders.test.ts @@ -3,19 +3,30 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { IMPORTED_HISTORY_SOURCES } from "@src/api/tauri/externalHistory"; -import { dataSourceConfigAtom } from "../../dataSourceConfigAtom"; -import { sessionsAtom } from "../atoms"; +import { + dataSourceConfigAtom, + externalSessionsEnabledAtom, +} from "../../dataSourceConfigAtom"; +import { + sessionLastLoadedAtom, + sessionLoadingAtom, + sessionsAtom, +} from "../atoms"; import { __TESTS_ONLY, loadMoreCategory, loadSessionRoster, + loadSessions, loadSidebarSessionById, loadSidebarSessions, loadSidebarSessionsByIds, refreshRecentNativeSessions, syncSidebarSessionRoster, } from "../loaders"; -import { sessionPaginationAtom } from "../paginationAtoms"; +import { + BASE_SESSION_LIST_CATEGORIES, + sessionPaginationAtom, +} from "../paginationAtoms"; const mocks = vi.hoisted(() => ({ externalHistorySidebarList: vi.fn(), @@ -824,6 +835,206 @@ describe("loadSidebarSessions", () => { } }); + it("shares one in-flight initial load between concurrent consumers", async () => { + let releaseExternalHistory!: () => void; + const externalHistoryPending = new Promise((resolve) => { + releaseExternalHistory = resolve; + }); + mocks.sessionAggregateList.mockResolvedValue({ sessions: [] }); + mocks.externalHistorySidebarList.mockImplementation( + async (request: { + requests: Array<{ + source: string; + buckets: Array<{ bucket: string }>; + }>; + }) => { + await externalHistoryPending; + return { + sources: request.requests.map((sourceRequest) => ({ + source: sourceRequest.source, + buckets: sourceRequest.buckets.map(({ bucket }) => ({ + bucket, + sessions: [], + hasMore: false, + })), + })), + }; + } + ); + + const firstLoad = loadSidebarSessions({ forceRefresh: true }); + const secondLoad = loadSidebarSessions({ forceRefresh: true }); + const thirdLoad = loadSidebarSessions(); + + expect(mocks.nativeSidebarSessionPage).toHaveBeenCalledTimes( + BASE_SESSION_LIST_CATEGORIES.length + ); + expect(mocks.externalHistorySidebarList).toHaveBeenCalledTimes(1); + + releaseExternalHistory(); + await Promise.all([firstLoad, secondLoad, thirdLoad]); + expect(mocks.persistSessions).toHaveBeenCalledTimes(1); + }); + + it("does not let a cache hit suppress a same-tick forced refresh", async () => { + mocks.store?.set(sessionLastLoadedAtom, Date.now()); + mocks.sessionAggregateList.mockResolvedValue({ sessions: [] }); + mocks.externalHistorySidebarList.mockImplementation( + async (request: { + requests: Array<{ + source: string; + buckets: Array<{ bucket: string }>; + }>; + }) => ({ + sources: request.requests.map((sourceRequest) => ({ + source: sourceRequest.source, + buckets: sourceRequest.buckets.map(({ bucket }) => ({ + bucket, + sessions: [], + hasMore: false, + })), + })), + }) + ); + + const cachedLoad = loadSidebarSessions(); + const forcedLoad = loadSidebarSessions({ forceRefresh: true }); + await Promise.all([cachedLoad, forcedLoad]); + + expect(mocks.nativeSidebarSessionPage).toHaveBeenCalledTimes( + BASE_SESSION_LIST_CATEGORIES.length + ); + expect(mocks.externalHistorySidebarList).toHaveBeenCalledTimes(1); + }); + + it("shares one flat-list request between concurrent hook consumers", async () => { + let release!: (value: { sessions: unknown[] }) => void; + mocks.sessionAggregateList.mockImplementation( + () => + new Promise<{ sessions: unknown[] }>((resolve) => { + release = resolve; + }) + ); + + const first = loadSessions(); + const second = loadSessions(); + + expect(mocks.sessionAggregateList).toHaveBeenCalledTimes(1); + release({ sessions: [] }); + await Promise.all([first, second]); + expect(mocks.persistSessions).toHaveBeenCalledTimes(1); + }); + + it("runs one forced flat-list refresh after an active non-forced load", async () => { + let releaseFirst!: (value: { sessions: unknown[] }) => void; + const staleSession = { + session_id: "stale", + name: "Stale", + status: "completed" as const, + created_at: "2026-07-01T00:00:00Z", + updated_at: "2026-07-01T00:00:00Z", + }; + const freshSession = { + ...staleSession, + session_id: "fresh", + name: "Fresh", + updated_at: "2026-07-02T00:00:00Z", + }; + mocks.sessionAggregateList + .mockImplementationOnce( + () => + new Promise<{ sessions: unknown[] }>((resolve) => { + releaseFirst = resolve; + }) + ) + .mockResolvedValueOnce({ sessions: [freshSession] }); + + const first = loadSessions(); + const forced = loadSessions({ forceRefresh: true }); + + expect(mocks.sessionAggregateList).toHaveBeenCalledTimes(1); + releaseFirst({ sessions: [staleSession] }); + await first; + await forced; + + expect(mocks.sessionAggregateList).toHaveBeenCalledTimes(2); + expect(mocks.store?.get(sessionsAtom)).toEqual([freshSession]); + expect(mocks.persistSessions).toHaveBeenCalledTimes(1); + expect(mocks.store?.get(sessionLoadingAtom)).toBe(false); + }); + + it("treats external-source configuration as part of the flat-list scope", async () => { + let releaseFirst!: (value: { sessions: unknown[] }) => void; + mocks.sessionAggregateList + .mockImplementationOnce( + () => + new Promise<{ sessions: unknown[] }>((resolve) => { + releaseFirst = resolve; + }) + ) + .mockResolvedValueOnce({ sessions: [] }); + + const first = loadSessions(); + mocks.store?.set(externalSessionsEnabledAtom, false); + const changedScope = loadSessions(); + + releaseFirst({ sessions: [] }); + await Promise.all([first, changedScope]); + + expect(mocks.sessionAggregateList).toHaveBeenCalledTimes(2); + expect(mocks.sessionAggregateList).toHaveBeenLastCalledWith( + expect.objectContaining({ includeExternalHistory: false }) + ); + }); + + it("runs a trailing load when the data-source scope changes in flight", async () => { + let releaseFirstLoad!: () => void; + const firstLoadPending = new Promise((resolve) => { + releaseFirstLoad = resolve; + }); + mocks.sessionAggregateList.mockResolvedValue({ sessions: [] }); + mocks.externalHistorySidebarList.mockImplementation( + async (request: { + requests: Array<{ + source: string; + buckets: Array<{ bucket: string }>; + }>; + }) => { + await firstLoadPending; + return { + sources: request.requests.map((sourceRequest) => ({ + source: sourceRequest.source, + buckets: sourceRequest.buckets.map(({ bucket }) => ({ + bucket, + sessions: [], + hasMore: false, + })), + })), + }; + } + ); + + const firstLoad = loadSidebarSessions({ forceRefresh: true }); + mocks.store?.set(dataSourceConfigAtom, { + warp: { enabled: false, frequency: "default", lastScannedAt: null }, + }); + const changedScopeLoad = loadSidebarSessions({ forceRefresh: true }); + + expect(mocks.externalHistorySidebarList).toHaveBeenCalledTimes(1); + releaseFirstLoad(); + await Promise.all([firstLoad, changedScopeLoad]); + + expect(mocks.nativeSidebarSessionPage).toHaveBeenCalledTimes( + BASE_SESSION_LIST_CATEGORIES.length * 2 + ); + expect(mocks.externalHistorySidebarList).toHaveBeenCalledTimes(2); + const trailingSources = + mocks.externalHistorySidebarList.mock.calls[1][0].requests.map( + ({ source }: { source: string }) => source + ); + expect(trailingSources).not.toContain("warp"); + }); + it("continues each external date bucket from its own offset", async () => { mocks.sessionAggregateList.mockResolvedValue({ sessions: [] }); mocks.externalHistorySidebarList.mockImplementation( diff --git a/src/store/session/sessionAtom/loaders.ts b/src/store/session/sessionAtom/loaders.ts index 439b8c4dd1..e096bc9c70 100644 --- a/src/store/session/sessionAtom/loaders.ts +++ b/src/store/session/sessionAtom/loaders.ts @@ -72,6 +72,8 @@ import type { Session, SessionStatus } from "./types"; const log = createLogger("SessionAtom"); const getStore = () => getInstrumentedStore(); +type SessionStore = ReturnType; + const BULK_CACHE_DURATION_MS = 5 * 60 * 1000; const DEFAULT_FLAT_LIST_PAGE_SIZE = 200; const RECENT_NATIVE_REFRESH_LIMIT = @@ -104,6 +106,20 @@ function exactSessionBatchLoadsForStore( return loads; } +interface FlatLoadFlight { + scopeKey: string; + forceRefresh: boolean; + generation: number; + promise: Promise; +} + +interface FlatLoadState { + generation: number; + flight?: FlatLoadFlight; +} + +const flatLoadStateByStore = new WeakMap(); + interface LoadSessionsOptions { repoPath?: string; orgId?: string; @@ -122,11 +138,41 @@ function loadSessionsCacheSignature(options?: LoadSessionsOptions): string { options?.projectSlug ?? "", options?.workItemId ?? "", options?.status ?? "", - options?.limit ?? "", - options?.offset ?? "", + options?.limit ?? DEFAULT_FLAT_LIST_PAGE_SIZE, + options?.offset ?? 0, ].join("\u001f"); } +interface FlatLoadScope { + cacheSignature: string; + disabledSources: string[]; + includeExternalHistory: boolean; + scopeKey: string; +} + +function getFlatLoadScope( + store: SessionStore, + options?: LoadSessionsOptions +): FlatLoadScope { + const disabledSources = Object.entries(store.get(dataSourceConfigAtom)) + .filter(([, config]) => config?.enabled === false) + .map(([sourceId]) => sourceId) + .sort(); + const includeExternalHistory = store.get(externalSessionsEnabledAtom); + const filterSignature = loadSessionsCacheSignature(options); + const scopeKey = JSON.stringify([ + filterSignature, + includeExternalHistory, + disabledSources, + ]); + return { + cacheSignature: scopeKey, + disabledSources, + includeExternalHistory, + scopeKey, + }; +} + function mergeSessions( prev: readonly Session[], incoming: readonly Session[] @@ -176,10 +222,10 @@ function replaceExternalHistorySourceFirstPage( } function setPaginationFor( + store: SessionStore, category: SessionListCategory, patch: Partial ) { - const store = getStore(); store.set(sessionPaginationAtom, (prev) => ({ ...prev, [category]: { ...prev[category], ...patch }, @@ -346,24 +392,13 @@ function mergeDateBucketPagination( return next; } -export const loadSessions = async (options?: LoadSessionsOptions) => { - const store = getStore(); - const { forceRefresh = false } = options || {}; - const cacheSignature = loadSessionsCacheSignature(options); - - const lastLoaded = store.get(sessionFlatListLastLoadedBySignatureAtom)[ - cacheSignature - ]; - const now = Date.now(); - - if ( - !forceRefresh && - lastLoaded && - now - lastLoaded < BULK_CACHE_DURATION_MS - ) { - return; - } - +async function performFlatSessionLoad( + store: SessionStore, + state: FlatLoadState, + generation: number, + scope: FlatLoadScope, + options?: LoadSessionsOptions +): Promise { store.set(sessionLoadingAtom, true); store.set(sessionErrorAtom, null); @@ -387,18 +422,14 @@ export const loadSessions = async (options?: LoadSessionsOptions) => { } : undefined; - const disabledSources = Object.entries(store.get(dataSourceConfigAtom)) - .filter(([, cfg]) => cfg?.enabled === false) - .map(([sourceId]) => sourceId); - const response = await sessionAggregateList({ ...filter, limit: filter?.limit ?? DEFAULT_FLAT_LIST_PAGE_SIZE, - includeExternalHistory: store.get(externalSessionsEnabledAtom), + includeExternalHistory: scope.includeExternalHistory, sortBy: filter?.sortBy ?? "updated_at", sortOrder: filter?.sortOrder ?? "desc", disabledExternalHistorySources: - disabledSources.length > 0 ? disabledSources : undefined, + scope.disabledSources.length > 0 ? scope.disabledSources : undefined, }); const fetched: Session[] = mergeGuestImportedSessions( @@ -409,20 +440,94 @@ export const loadSessions = async (options?: LoadSessionsOptions) => { (sessionB.updated_at || "").localeCompare(sessionA.updated_at || "") ); + if (state.generation !== generation) return; + store.set(sessionsAtom, fetched); persistSessions(fetched); store.set(sessionFlatListLastLoadedBySignatureAtom, (prev) => ({ ...prev, - [cacheSignature]: now, + [scope.cacheSignature]: Date.now(), })); } catch (error) { + if (state.generation !== generation) return; log.error("[SessionAtom] Failed to load sessions:", error); store.set( sessionErrorAtom, error instanceof Error ? error.message : "Failed to load sessions" ); } finally { - store.set(sessionLoadingAtom, false); + if (state.generation === generation) { + store.set(sessionLoadingAtom, false); + } + } +} + +/** + * Coordinate flat-list requests across every hook instance using this store. + * + * Equal scopes share one request. A stronger forced refresh or changed scope + * supersedes the current generation, waits for it to release the IPC slot, + * and then performs one trailing request. Superseded responses never write. + */ +export const loadSessions = async ( + options?: LoadSessionsOptions +): Promise => { + const store = getStore(); + const forceRefresh = options?.forceRefresh ?? false; + const scope = getFlatLoadScope(store, options); + const lastLoaded = store.get(sessionFlatListLastLoadedBySignatureAtom)[ + scope.cacheSignature + ]; + + if ( + !forceRefresh && + lastLoaded && + Date.now() - lastLoaded < BULK_CACHE_DURATION_MS + ) { + return; + } + + let state = flatLoadStateByStore.get(store); + if (!state) { + state = { generation: 0 }; + flatLoadStateByStore.set(store, state); + } + + const current = state.flight; + if (current) { + const currentSatisfiesRequest = + current.scopeKey === scope.scopeKey && + (!forceRefresh || current.forceRefresh); + if (currentSatisfiesRequest) return current.promise; + + // Fence the old response immediately, before waiting for its IPC call. + state.generation += 1; + await current.promise; + return loadSessions(options); + } + + const generation = state.generation + 1; + state.generation = generation; + const promise = performFlatSessionLoad( + store, + state, + generation, + scope, + options + ); + state.flight = { + scopeKey: scope.scopeKey, + forceRefresh, + generation, + promise, + }; + + try { + await promise; + } finally { + if (state.flight?.promise === promise) { + state.flight = undefined; + } } }; @@ -479,10 +584,14 @@ async function loadCategoryPage( interface SidebarLoadOptions { pageSize?: number; forceRefresh?: boolean; + /** Internal data-source identity used to serialize scope changes. */ + scopeKey?: string; } -const performSidebarSessionLoad = async (options?: SidebarLoadOptions) => { - const store = getStore(); +const performSidebarSessionLoad = async ( + store: SessionStore, + options?: SidebarLoadOptions +) => { const pageSize = options?.pageSize ?? SESSION_SIDEBAR_PAGE_SIZE; const { forceRefresh = false } = options ?? {}; @@ -513,12 +622,12 @@ const performSidebarSessionLoad = async (options?: SidebarLoadOptions) => { }; for (const category of SESSION_LIST_CATEGORIES) { - setPaginationFor(category, { phase: "loading" }); + setPaginationFor(store, category, { phase: "loading" }); } const enabledCategories = SESSION_LIST_CATEGORIES.filter((category) => { if (!isCategoryDisabled(category)) return true; - setPaginationFor(category, { + setPaginationFor(store, category, { sessionIds: [], cursor: null, phase: "exhausted", @@ -546,7 +655,7 @@ const performSidebarSessionLoad = async (options?: SidebarLoadOptions) => { // authoritative page replaces only `sessionIds`; older cached entities // remain available for active/deep-link overlays. store.set(sessionsAtom, (prev) => mergeSessions(prev, primarySessions)); - setPaginationFor(category, { + setPaginationFor(store, category, { sessionIds, cursor: nextCursor ?? null, phase: hasMore ? "ready" : "exhausted", @@ -564,7 +673,7 @@ const performSidebarSessionLoad = async (options?: SidebarLoadOptions) => { } catch (error) { log.warn(`[SessionAtom] ${category} initial page failed:`, error); if (generation === currentSidebarRosterGeneration(store)) { - setPaginationFor(category, { + setPaginationFor(store, category, { cursor: null, phase: "error", generation, @@ -586,7 +695,7 @@ const performSidebarSessionLoad = async (options?: SidebarLoadOptions) => { // independently authoritative, so for them the blanking is total. const markImportedStreamFailed = (category: SessionListCategory) => { if (generation !== currentSidebarRosterGeneration(store)) return; - setPaginationFor(category, { cursor: null, phase: "error" }); + setPaginationFor(store, category, { cursor: null, phase: "error" }); }; const importedTask = (async () => { @@ -636,6 +745,7 @@ function mergeSidebarLoadOptions( current: SidebarLoadOptions | null, requested: SidebarLoadOptions ): SidebarLoadOptions { + const scopeKey = requested.scopeKey ?? current?.scopeKey; return { pageSize: Math.max( current?.pageSize ?? SESSION_SIDEBAR_PAGE_SIZE, @@ -643,6 +753,7 @@ function mergeSidebarLoadOptions( ), forceRefresh: (current?.forceRefresh ?? false) || (requested.forceRefresh ?? false), + ...(scopeKey === undefined ? {} : { scopeKey }), }; } @@ -654,6 +765,7 @@ function sidebarLoadCovers( const activePageSize = active.pageSize ?? SESSION_SIDEBAR_PAGE_SIZE; const requestedPageSize = requested.pageSize ?? SESSION_SIDEBAR_PAGE_SIZE; return ( + active.scopeKey === requested.scopeKey && activePageSize >= requestedPageSize && ((active.forceRefresh ?? false) || !(requested.forceRefresh ?? false)) ); @@ -695,13 +807,56 @@ function createSidebarLoadCoordinator( } /** - * One process-wide session-roster loader. Overlapping mounts/refreshes join the - * active read; a stronger request (forced or larger page) is merged into one - * follow-up pass instead of starting a parallel category fan-out. + * One session-roster loader per Jotai store. Overlapping mounts/refreshes join + * the active read; a stronger request (forced, larger, or a changed data-source + * scope) is merged into one follow-up pass instead of starting a parallel + * category fan-out. WeakMap ownership prevents cross-window/store leakage. */ -export const loadSessionRoster = createSidebarLoadCoordinator( - performSidebarSessionLoad -); +const sidebarLoadCoordinatorByStore = new WeakMap< + SessionStore, + ReturnType +>(); +const sidebarLoadScopeByStore = new WeakMap(); + +function getSidebarLoadScopeKey(store: SessionStore): string { + const disabledSources = Object.entries(store.get(dataSourceConfigAtom)) + .filter(([, config]) => config?.enabled === false) + .map(([sourceId]) => sourceId) + .sort(); + return JSON.stringify([ + store.get(externalSessionsEnabledAtom), + disabledSources, + ]); +} + +function sidebarLoadCoordinatorForStore( + store: SessionStore +): ReturnType { + let coordinator = sidebarLoadCoordinatorByStore.get(store); + if (!coordinator) { + coordinator = createSidebarLoadCoordinator((options) => + performSidebarSessionLoad(store, options) + ); + sidebarLoadCoordinatorByStore.set(store, coordinator); + } + return coordinator; +} + +export const loadSessionRoster = ( + options: SidebarLoadOptions = {} +): Promise => { + const store = getStore(); + const scopeKey = getSidebarLoadScopeKey(store); + const previousScopeKey = sidebarLoadScopeByStore.get(store); + const scopeChanged = + previousScopeKey !== undefined && previousScopeKey !== scopeKey; + sidebarLoadScopeByStore.set(store, scopeKey); + return sidebarLoadCoordinatorForStore(store)({ + ...options, + forceRefresh: (options.forceRefresh ?? false) || scopeChanged, + scopeKey, + }); +}; /** * Compatibility alias for callers outside the roster surfaces. New Sidebar @@ -869,7 +1024,7 @@ export const loadMoreCategory = async ( const generation = currentSidebarRosterGeneration(store) || nextSidebarRosterGeneration(store); - setPaginationFor(category, { phase: "loading" }); + setPaginationFor(store, category, { phase: "loading" }); try { const { sessions, hasMore, nextCursor, dateBuckets } = @@ -919,7 +1074,7 @@ export const loadMoreCategory = async ( ? returnedIds : [...current.sessionIds, ...newSessionIds]; store.set(sessionsAtom, (prev) => mergeSessions(prev, primarySessions)); - setPaginationFor(category, { + setPaginationFor(store, category, { sessionIds, cursor: imported ? null : (nextCursor ?? current.cursor), phase: hasMore ? "ready" : "exhausted", @@ -939,7 +1094,7 @@ export const loadMoreCategory = async ( } catch (error) { log.warn(`[SessionAtom] loadMoreCategory(${category}) failed:`, error); if (generation === currentSidebarRosterGeneration(store)) { - setPaginationFor(category, { phase: "error", generation }); + setPaginationFor(store, category, { phase: "error", generation }); } return { category, diff --git a/src/util/platform/tauri/fileSearch.test.ts b/src/util/platform/tauri/fileSearch.test.ts new file mode 100644 index 0000000000..464f2d9857 --- /dev/null +++ b/src/util/platform/tauri/fileSearch.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from "vitest"; + +import { shouldPrewarmFileIndex } from "./fileSearch"; + +describe("shouldPrewarmFileIndex", () => { + it("allows visible and non-DOM callers", () => { + expect(shouldPrewarmFileIndex("visible")).toBe(true); + expect(shouldPrewarmFileIndex(undefined)).toBe(true); + }); + + it("skips proactive work for hidden windows", () => { + expect(shouldPrewarmFileIndex("hidden")).toBe(false); + }); +}); diff --git a/src/util/platform/tauri/fileSearch.ts b/src/util/platform/tauri/fileSearch.ts index dad6127b03..10802d6a6e 100644 --- a/src/util/platform/tauri/fileSearch.ts +++ b/src/util/platform/tauri/fileSearch.ts @@ -16,6 +16,7 @@ import type { SearchResultItem } from "@src/scaffold/ContextMenu/types"; import { ensureTauriReady, invokeTauri, isTauriReady } from "./init"; const log = createLogger("FileSearch"); +const prewarmRequests = new Map>(); // ============================================ // Types @@ -137,17 +138,32 @@ export async function indexProjectFiles( */ export async function prewarmFileIndex(rootPath: string): Promise { if (!isTauriReady()) return 0; - - try { - const count = await invokeTauri("prewarm_file_index", { - rootPath, + if (!shouldPrewarmFileIndex(globalThis.document?.visibilityState)) return 0; + + const existingRequest = prewarmRequests.get(rootPath); + if (existingRequest) return existingRequest; + + const request = invokeTauri("prewarm_file_index", { rootPath }) + .catch((error) => { + // Non-fatal — search will still work, just cold on first use. + log.warn("[FileSearch] Prewarm failed (non-fatal):", error); + return 0; + }) + .finally(() => { + if (prewarmRequests.get(rootPath) === request) { + prewarmRequests.delete(rootPath); + } }); - return count; - } catch (error) { - // Non-fatal — search will still work, just cold on first use. - log.warn("[FileSearch] Prewarm failed (non-fatal):", error); - return 0; - } + + prewarmRequests.set(rootPath, request); + return request; +} + +/** Hidden windows do not spend CPU pre-walking projects. */ +export function shouldPrewarmFileIndex( + visibilityState: DocumentVisibilityState | undefined +): boolean { + return visibilityState !== "hidden"; } /** @@ -168,6 +184,23 @@ export async function clearFileIndexCache(): Promise { } } +/** + * Mark one workspace's file-path index stale without starting a scan. + * The next foreground prewarm or search rebuilds it on demand. + */ +export async function invalidateFileIndexCache( + rootPath: string +): Promise { + if (!isTauriReady()) return; + + try { + await invokeTauri("invalidate_file_index_cache", { rootPath }); + } catch (error) { + log.error("[FileSearch] Failed to invalidate cache:", error); + throw error; + } +} + // ============================================ // Helper Functions // ============================================