diff --git a/docs/architecture-audit-2026-07-23/AsyncResourceLifecycle.md b/docs/architecture-audit-2026-07-23/AsyncResourceLifecycle.md new file mode 100644 index 0000000000..f299b22a74 --- /dev/null +++ b/docs/architecture-audit-2026-07-23/AsyncResourceLifecycle.md @@ -0,0 +1,109 @@ +# Architecture Audit — Async Resource Lifecycle + +**Scope:** shared async-resource state, visibility-aware polling, and the migrated TypeScript query owners. +**Date:** 2026-07-23 +**Auditor:** ORGII implementation session + +## Acceptance criteria + +- One owner for `data / error / loading / refreshing / reload` state. +- Equal in-flight scope loads coalesce; explicit refresh starts a new generation. +- A completion from an old repo, workspace, project, filter, language, or webview cannot commit. +- Disabled and changed scopes cannot display data from the previous scope. +- Background polling pauses while hidden, never overlaps, and stops cleanly. +- Mutation/action loading remains separate from query loading. +- Caches are bounded and include the complete resource identity. +- TypeScript, focused lint, lifecycle tests, and `git diff --check` pass. + +## Layer 1 — Compilation correctness + +- `pnpm run typecheck`: passed. +- Focused ESLint across all changed frontend files: passed. +- Focused Vitest run: 10 files and 118 tests passed. +- `git diff --check`: passed. + +## Layer 2 — Dead code and structural deduplication + +- Removed the unused `useAsyncData` abstraction. +- `useAsyncResource` is now the single generic owner for request state and generation fencing. +- `useVisibilityPolledData` composes the same owner instead of maintaining a second polling-specific state machine. +- Duplicate initial-load/manual-refresh implementations were removed from the migrated hooks. + +## Layer 3 — Naming consistency + +| Term | Meaning | Verdict | +| --- | --- | --- | +| `scopeKey` | Complete identity of the visible resource | Explicit and consistent | +| `reload` | Load or background-revalidate, joining an equal in-flight generation | Explicit | +| `refresh` | User-requested superseding generation | Explicit | +| `loading` | Initial load or foreground refresh | Kept for consumer compatibility | +| `refreshing` | Existing data is retained during foreground refresh | Separate state, not overloaded with action progress | +| `operationLoading` / `gatewayLoading` | Explicit user mutation in progress | Correctly remains outside the query resource | + +## Layer 4 — Semantic overloading + +- Query state and mutation state remain separate in Stash, Gateway, Work Item, and provider flows. +- `background` controls presentation only; it does not weaken generation checks. +- `publish` means an intermediate current-scope cache value, not completion. +- `setData` is limited to optimistic/current-resource updates and cannot write while the resource is disabled. + +## Layer 5 — Default branch analysis + +| Condition | Result | +| --- | --- | +| `enabled === false` or `scopeKey === null` | Reset to initial data/status and supersede active work | +| Same automatic scope already in flight | Join the existing promise | +| Manual refresh | Supersede and start a new generation | +| Scope changes | Hide old data immediately and reject late completion | +| Fetch rejects | Preserve current-scope data, expose normalized error | +| Hidden document | Retain no polling timer | +| Visibility returns | Run one immediate catch-up pass | +| Poll stops during DOM dirty-check | Effect-local active fence prevents a post-teardown reload | + +## Layer 6 — Cross-domain concept leakage + +- `useAsyncResource`, `LatestScopedTask`, and `startVisibilityAwarePoll` contain no project, Git, LSP, provider, or webview domain imports. +- Domain fetchers own payload parsing and cache policy; the generic lifecycle layer owns only scheduling and commit eligibility. +- Tauri and HTTP command names remain at their domain call sites. + +## Layer 7 — New developer confusion test + +- The hook contract documents the difference between automatic load, manual refresh, background reload, and intermediate cache publish. +- A resource's visible state is derived from the current `scopeKey`; consumers do not need local cancellation refs. +- Action states are visibly named and remain local where they represent distinct user operations. + +## Layer 8 — Wire protocol and serialization + +- No backend command schema or wire payload was changed. +- Serialized scope keys are frontend-only coordinator identities and are parsed by the matching local fetcher. +- Scope keys include all relevant identity fields, including repo ID/path, connection/team/surface, filter, language, and webview label/depth. + +## Layer 9 — Init parity + +| Entry path | Resource owner | Generation guard | Error normalization | +| --- | --- | ---: | ---: | +| Automatic first load | `useAsyncResource` | yes | yes | +| Manual refresh | `useAsyncResource.refresh` | yes, superseding | yes | +| Background poll | `reload({ background: true })` | yes | yes | +| Cache then live result | `context.publish` + final return | yes | yes | +| Disabled/unmounted scope | effect cleanup | yes | n/a | + +## Layer 10 — Resolver symmetry + +- Every migrated resource uses the same scope value for loading, visibility, stale-result rejection, and optimistic updates. +- Cached and live values use the same resource identity. +- The commit-diff cache now includes repository identity as well as commit SHA, eliminating cross-repository collisions. + +## Systematic sweep + +- Searched query hooks for repeated `loading/error/data` owners and manual cancellation/request-ID patterns. +- Searched active runtime code for `setInterval`; migrated non-critical IPC polling peers. +- Kept animation clocks, debounces, durable persistence heartbeats, editor/document FSMs, user-triggered searches, and mutation progress states with their specialized owners. +- Remaining literal `setInterval` hits in the audited directories are UI clocks/simulations or domain-specific lifecycles, not duplicate query polling. + +## Completion verdict + +- One teardown issue found during audit was fixed: DOM dirty polling now cannot schedule a tree reload after its effect has stopped. +- Relevant architecture layers 1–10 were checked; backend init, schema migration, and resolver-chain changes were not applicable because no backend or wire contract changed. + +**Architecture verdict: pass for the audited async-resource and polling scope.** diff --git a/docs/frontend-ui-audit-2026-07-23/AsyncResourceConsumers.md b/docs/frontend-ui-audit-2026-07-23/AsyncResourceConsumers.md new file mode 100644 index 0000000000..d2626be936 --- /dev/null +++ b/docs/frontend-ui-audit-2026-07-23/AsyncResourceConsumers.md @@ -0,0 +1,42 @@ +# Frontend UI Audit — Async Resource Consumers + +**Files:** `src/engines/ChatPanel/panels/ProjectPanelView.tsx`, `src/modules/ProjectManager/LinearProjects/useLinearIndexData.tsx` +**Date:** 2026-07-23 +**Auditor:** ORGII implementation session + +The diff in both files changes data ownership only. It does not add or modify rendered JSX, class names, interactive elements, or layout. + +## D1 — Raw HTML vs Design System + +| Line | Element | Verdict | Reason | Suggested change | +| --- | --- | --- | --- | --- | +| Changed hunks | none | keep | The refactor introduces no raw interactive or structural HTML. | — | + +## D2 — Arbitrary Tailwind Value vs Token + +| Line | Value | Verdict | Reason | Suggested change | +| --- | --- | --- | --- | --- | +| Changed hunks | none | keep | No Tailwind or CSS-variable class changed. | — | + +## D3 — Hardcoded Sizes / Colors + +| Line | Value | Verdict | Reason | Suggested change | +| --- | --- | --- | --- | --- | +| Changed hunks | none | keep | No size or color literal changed. | — | + +## D4 — Accessibility + +| Line | Element | Verdict | Reason | Suggested change | +| --- | --- | --- | --- | --- | +| Changed hunks | none | keep | No rendered element or interaction contract changed. | — | + +## D5 — Visual Patterns Observed + +- No new visual pattern was introduced. +- The shared abstraction is a data-lifecycle hook and is not a design-system component candidate. + +## Summary + +- 0 fixes recommended +- 0 kept exceptions requiring future review +- 0 abstract UI candidates diff --git a/docs/org2-performance-guard-2026-07-23/AsyncResourceLifecycle.md b/docs/org2-performance-guard-2026-07-23/AsyncResourceLifecycle.md new file mode 100644 index 0000000000..549985ff24 --- /dev/null +++ b/docs/org2-performance-guard-2026-07-23/AsyncResourceLifecycle.md @@ -0,0 +1,35 @@ +# Performance Guard — Async Resource Lifecycle + +**Scope:** migrated resource fetches, background polling, request caches, and multi-scope lifecycle. +**Date:** 2026-07-23 + +## Lifecycle matrix + +| State | Required behavior | Audited behavior | +| --- | --- | --- | +| Visible and active | Fetch on demand at configured cadence | Recursive polling; next delay starts after settlement | +| Visible and idle | Avoid full reload unless dirty or scheduled safety refresh | DOM uses dirty-check; other retained polls are bounded safety refreshes | +| Hidden | No non-critical polling timer | Timer cleared; visibility return triggers one catch-up pass | +| Scope switch | Previous data and completion cannot appear | Complete scope key plus generation fence | +| Unmount/disable | Stop timer/listener and reject late commits | Poll controller cleanup plus coordinator supersede | +| Offline/error | Set current resource error without a retry storm | No automatic tight retry; configured cadence resumes | +| Repeated mount | No app-lifetime accumulation | Per-hook coordinator owns one active promise; listeners/timers dispose | + +## Findings and evidence + +| Area | Verdict | Evidence | Change or reason kept | Verification | +| --- | --- | --- | --- | --- | +| Background work | fix | DOM, Inspector, Console, Network, LSP, Git auto-fetch, and Gateway used or consumed polling | Replaced non-critical intervals with visibility-aware non-overlapping recursive polling; DOM teardown received an additional active fence | Visibility controller tests plus focused lifecycle review | +| Memory | fix/keep | Resource retains one state and one active promise; Console cache is 10 sessions × 500 rows, Network is 10 × 200, commit cache is 50 | Preserved existing caps; removed duplicate per-resource state; no new unbounded collection | Unit tests, code inspection | +| Scope/isolation | fix | Previous implementations used local mounted/cancelled flags or incomplete commit cache identity | Complete scope keys and generations now gate every commit; commit cache includes repo identity | Stale-filter, stale-scope, superseding-refresh tests | +| Rendering/hot path | fix | Foreground state was repeatedly toggled by background refreshes | Background reload retains data and avoids spinner flashes; derived grouping remains memoized | Async-resource and polling hook tests | + +## Verification + +- `pnpm run typecheck`: passed. +- Focused ESLint: passed. +- Focused Vitest: 10 files, 118 tests passed. +- `git diff --check`: passed. +- Real Tauri visible/hidden IPC and CPU measurement was not run in this audit environment. + +**Performance verdict: blocked only on real Tauri runtime measurement; static lifecycle gates, compilation, lint, and focused regression tests pass.** 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/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 db964afa65..4239c9a6c7 100644 --- a/src-tauri/src/commands/handler_list.inc +++ b/src-tauri/src/commands/handler_list.inc @@ -220,6 +220,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 9cadddbaa8..337821e812 100644 --- a/src/api/http/project/client.ts +++ b/src/api/http/project/client.ts @@ -38,6 +38,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 // ============================================ @@ -389,18 +411,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( @@ -594,13 +626,35 @@ 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` 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/hooks/async/index.ts b/src/hooks/async/index.ts index a2a538279f..b77495af7e 100644 --- a/src/hooks/async/index.ts +++ b/src/hooks/async/index.ts @@ -1,9 +1,13 @@ -// Async data hooks export { - useAsyncData, - useAsyncAction, - type UseAsyncDataOptions, - type UseAsyncDataReturn, - type UseAsyncActionOptions, - type UseAsyncActionReturn, -} from "./useAsyncData"; + useAsyncResource, + type AsyncResourceFetchContext, + type AsyncResourceReloadOptions, + type AsyncResourceStatus, + type UseAsyncResourceOptions, + type UseAsyncResourceResult, +} from "./useAsyncResource"; +export { + useVisibilityPolledData, + type UseVisibilityPolledDataOptions, + type UseVisibilityPolledDataResult, +} from "./useVisibilityPolledData"; diff --git a/src/hooks/async/useAsyncData.ts b/src/hooks/async/useAsyncData.ts deleted file mode 100644 index 1d4ae0221a..0000000000 --- a/src/hooks/async/useAsyncData.ts +++ /dev/null @@ -1,237 +0,0 @@ -/** - * useAsyncData Hook - * - * Generic hook for async data fetching with loading/error state management. - * Consolidates the common pattern found across 60+ hooks in the codebase. - * - * Features: - * - Unified loading/error/data state management - * - Auto-load on mount with dependency tracking - * - Success/error callbacks - * - Manual refresh capability - * - Type-safe with generics - * - * @example - * const { data, loading, error, refresh } = useAsyncData({ - * fetcher: () => api.fetchItems(), - * initialData: [], - * errorPrefix: "Failed to load items", - * }); - */ -import { - type Dispatch, - type SetStateAction, - useCallback, - useEffect, - useState, -} from "react"; - -import { useMounted } from "@src/hooks/lifecycle/useMounted"; - -// ============================================ -// Type Definitions -// ============================================ - -export interface UseAsyncDataOptions { - /** Async function to fetch data */ - fetcher: () => Promise; - /** Auto-load on mount (default: true) */ - autoLoad?: boolean; - /** Dependencies that trigger refetch when changed */ - deps?: unknown[]; - /** Success callback */ - onSuccess?: (data: T) => void; - /** Error callback */ - onError?: (error: Error) => void; - /** Initial data value */ - initialData?: T; - /** Error message prefix for generic errors */ - errorPrefix?: string; - /** Skip fetch if condition is false */ - enabled?: boolean; -} - -export interface UseAsyncDataReturn { - /** The fetched data */ - data: T; - /** Loading state */ - loading: boolean; - /** Error message (null if no error) */ - error: string | null; - /** Manually trigger a refresh */ - refresh: () => Promise; - /** Directly update the data state */ - setData: Dispatch>; - /** Clear the error state */ - clearError: () => void; -} - -// ============================================ -// Hook Implementation -// ============================================ - -export function useAsyncData( - options: UseAsyncDataOptions -): UseAsyncDataReturn { - const { - fetcher, - autoLoad = true, - deps = [], - onSuccess, - onError, - initialData, - errorPrefix = "Failed to load data", - enabled = true, - } = options; - - // State - const [data, setData] = useState(initialData as T); - const [loading, setLoading] = useState(false); - const [error, setError] = useState(null); - - const mountedRef = useMounted(); - - // Refresh function - const refresh = useCallback(async () => { - if (!enabled) { - return; - } - - setLoading(true); - setError(null); - - try { - const result = await fetcher(); - - if (mountedRef.current) { - setData(result); - onSuccess?.(result); - } - } catch (err) { - if (mountedRef.current) { - const message = - err instanceof Error ? err.message : `${errorPrefix}: ${String(err)}`; - setError(message); - onError?.(err instanceof Error ? err : new Error(message)); - } - } finally { - if (mountedRef.current) { - setLoading(false); - } - } - }, [fetcher, enabled, errorPrefix, onSuccess, onError, mountedRef]); - - // Clear error helper - const clearError = useCallback(() => { - setError(null); - }, []); - - // Auto-load on mount and when deps change - useEffect(() => { - if (autoLoad && enabled) { - refresh(); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [autoLoad, enabled, ...deps]); - - return { - data, - loading, - error, - refresh, - setData, - clearError, - }; -} - -// ============================================ -// Utility: useAsyncAction (for mutations) -// ============================================ - -export interface UseAsyncActionOptions { - /** Success callback */ - onSuccess?: () => void; - /** Error callback */ - onError?: (error: Error) => void; - /** Error message prefix */ - errorPrefix?: string; -} - -export interface UseAsyncActionReturn { - /** Execute the action */ - execute: (...args: TArgs) => Promise; - /** Loading state */ - loading: boolean; - /** Error message */ - error: string | null; - /** Clear error */ - clearError: () => void; -} - -/** - * Hook for async actions/mutations (create, update, delete operations) - * - * @example - * const { execute: createItem, loading } = useAsyncAction( - * async (name: string) => { - * return await api.createItem({ name }); - * }, - * { onSuccess: refresh } - * ); - */ -export function useAsyncAction( - action: (...args: TArgs) => Promise, - options: UseAsyncActionOptions = {} -): UseAsyncActionReturn { - const { onSuccess, onError, errorPrefix = "Action failed" } = options; - - const [loading, setLoading] = useState(false); - const [error, setError] = useState(null); - - const mountedRef = useMounted(); - - const execute = useCallback( - async (...args: TArgs): Promise => { - setLoading(true); - setError(null); - - try { - const result = await action(...args); - - if (mountedRef.current) { - onSuccess?.(); - } - - return result; - } catch (err) { - if (mountedRef.current) { - const message = - err instanceof Error - ? err.message - : `${errorPrefix}: ${String(err)}`; - setError(message); - onError?.(err instanceof Error ? err : new Error(message)); - } - return null; - } finally { - if (mountedRef.current) { - setLoading(false); - } - } - }, - [action, errorPrefix, onSuccess, onError, mountedRef] - ); - - const clearError = useCallback(() => { - setError(null); - }, []); - - return { - execute, - loading, - error, - clearError, - }; -} - -export default useAsyncData; diff --git a/src/hooks/async/useAsyncResource.test.ts b/src/hooks/async/useAsyncResource.test.ts new file mode 100644 index 0000000000..fc3a98fffc --- /dev/null +++ b/src/hooks/async/useAsyncResource.test.ts @@ -0,0 +1,296 @@ +// @vitest-environment jsdom +import { act, createElement, useEffect } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { + type UseAsyncResourceResult, + useAsyncResource, +} from "./useAsyncResource"; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, reject, resolve }; +} + +async function flush(): Promise { + await act(async () => { + await Promise.resolve(); + }); +} + +describe("useAsyncResource", () => { + let container: HTMLDivElement; + let root: Root; + let current: UseAsyncResourceResult; + const actEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; + }; + + function Harness({ + autoLoad = true, + enabled = true, + fetcher, + initialData = "", + initialStatus = "idle", + scopeKey, + }: { + autoLoad?: boolean; + enabled?: boolean; + fetcher: Parameters>[0]["fetcher"]; + initialData?: string; + initialStatus?: "idle" | "ready"; + scopeKey: string | null; + }) { + const result = useAsyncResource({ + autoLoad, + enabled, + fetcher, + initialData, + initialStatus, + scopeKey, + }); + useEffect(() => { + current = result; + }, [result]); + return createElement("div", { + "data-error": result.error ?? "", + "data-loading": String(result.loading), + "data-refreshing": String(result.refreshing), + "data-status": result.status, + "data-value": result.data, + }); + } + + beforeAll(() => { + actEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + }); + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + afterAll(() => { + Reflect.deleteProperty(actEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + it("loads and exposes one cohesive resource state", async () => { + const request = deferred(); + const fetcher = vi.fn().mockReturnValue(request.promise); + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + + expect(container.firstElementChild?.getAttribute("data-status")).toBe( + "loading" + ); + request.resolve("loaded"); + await flush(); + + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "loaded" + ); + expect(container.firstElementChild?.getAttribute("data-status")).toBe( + "ready" + ); + }); + + it("drops a late response and hides old data after switching scope", async () => { + const first = deferred(); + const second = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockImplementation((scope) => + scope === "a" ? first.promise : second.promise + ); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "b" }))); + expect(container.firstElementChild?.getAttribute("data-value")).toBe(""); + + second.resolve("new"); + await flush(); + first.resolve("old"); + await flush(); + + expect(container.firstElementChild?.getAttribute("data-value")).toBe("new"); + }); + + it("starts a new generation for manual refresh and preserves visible data", async () => { + const stale = deferred(); + const fresh = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockReturnValueOnce(stale.promise) + .mockReturnValueOnce(fresh.promise); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + stale.resolve("initial"); + await flush(); + + let refresh!: Promise; + act(() => { + refresh = current.refresh(); + }); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "initial" + ); + expect(container.firstElementChild?.getAttribute("data-refreshing")).toBe( + "true" + ); + + fresh.resolve("fresh"); + await act(async () => refresh); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "fresh" + ); + }); + + it("prevents an active initial load from overwriting a manual refresh", async () => { + const stale = deferred(); + const fresh = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockReturnValueOnce(stale.promise) + .mockReturnValueOnce(fresh.promise); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + let refresh!: Promise; + act(() => { + refresh = current.refresh(); + }); + + fresh.resolve("fresh"); + await act(async () => refresh); + stale.resolve("stale"); + await flush(); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "fresh" + ); + }); + + it("publishes cache data before the current live request settles", async () => { + const live = deferred(); + const fetcher = vi.fn( + async (_scopeKey: string, context: { publish(data: string): void }) => { + context.publish("cached"); + return live.promise; + } + ); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "cached" + ); + expect(container.firstElementChild?.getAttribute("data-status")).toBe( + "ready" + ); + + live.resolve("live"); + await flush(); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "live" + ); + }); + + it("can expose seeded cache data without starting a request", () => { + const fetcher = vi.fn().mockResolvedValue("unused"); + act(() => + root.render( + createElement(Harness, { + autoLoad: false, + fetcher, + initialData: "cached", + initialStatus: "ready", + scopeKey: "a", + }) + ) + ); + + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "cached" + ); + expect(container.firstElementChild?.getAttribute("data-status")).toBe( + "ready" + ); + expect(fetcher).not.toHaveBeenCalled(); + }); + + it("joins equal non-superseding loads", async () => { + const request = deferred(); + const fetcher = vi.fn().mockReturnValue(request.promise); + act(() => + root.render( + createElement(Harness, { + autoLoad: false, + fetcher, + scopeKey: "a", + }) + ) + ); + + let first!: Promise; + let second!: Promise; + act(() => { + first = current.reload(); + second = current.reload(); + }); + expect(fetcher).toHaveBeenCalledTimes(1); + + request.resolve("joined"); + await act(async () => Promise.all([first, second])); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "joined" + ); + }); + + it("recovers from error and resets when disabled", async () => { + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce("recovered"); + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + await flush(); + expect(container.firstElementChild?.getAttribute("data-error")).toBe( + "offline" + ); + + await act(async () => current.refresh()); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "recovered" + ); + + act(() => + root.render( + createElement(Harness, { + enabled: false, + fetcher, + scopeKey: "a", + }) + ) + ); + expect(container.firstElementChild?.getAttribute("data-value")).toBe(""); + expect(container.firstElementChild?.getAttribute("data-status")).toBe( + "idle" + ); + }); +}); diff --git a/src/hooks/async/useAsyncResource.ts b/src/hooks/async/useAsyncResource.ts new file mode 100644 index 0000000000..e08e5cf930 --- /dev/null +++ b/src/hooks/async/useAsyncResource.ts @@ -0,0 +1,234 @@ +import { + type Dispatch, + type SetStateAction, + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from "react"; + +import { LatestScopedTask } from "@src/util/core/latestScopedTask"; + +export type AsyncResourceStatus = + | "idle" + | "loading" + | "ready" + | "refreshing" + | "error"; + +interface AsyncResourceState { + data: T; + error: string | null; + scopeKey: string | null; + status: AsyncResourceStatus; +} + +export interface UseAsyncResourceOptions { + autoLoad?: boolean; + enabled?: boolean; + fetcher: ( + scopeKey: string, + context: AsyncResourceFetchContext + ) => Promise; + initialData: T; + initialStatus?: "idle" | "ready"; + scopeKey: string | null; +} + +export interface AsyncResourceFetchContext { + cause: "background" | "load" | "refresh"; + isCurrent(): boolean; + /** Commit an intermediate cache/stale-while-revalidate value if still current. */ + publish(data: T, options?: { keepLoading?: boolean }): void; +} + +export interface AsyncResourceReloadOptions { + /** Keep the current loading indicator unchanged, for background revalidation. */ + background?: boolean; + /** Start a new generation instead of joining an equal in-flight scope. */ + supersede?: boolean; +} + +export interface UseAsyncResourceResult { + data: T; + error: string | null; + loading: boolean; + refreshing: boolean; + reload: (options?: AsyncResourceReloadOptions) => Promise; + refresh: () => Promise; + setData: Dispatch>; + status: AsyncResourceStatus; +} + +/** + * Own one scope-fenced async resource. + * + * Equal automatic loads share an in-flight promise. Manual refreshes start a + * new generation, and every completion verifies that its scope/generation is + * still current before committing state. + */ +export function useAsyncResource({ + autoLoad = true, + enabled = true, + fetcher, + initialData, + initialStatus = "idle", + scopeKey, +}: UseAsyncResourceOptions): UseAsyncResourceResult { + const coordinator = useMemo(() => new LatestScopedTask(), []); + const initialDataRef = useRef(initialData); + initialDataRef.current = initialData; + const initialStatusRef = useRef(initialStatus); + initialStatusRef.current = initialStatus; + const [state, setState] = useState>({ + data: initialData, + error: null, + scopeKey: null, + status: initialStatus, + }); + + const reload = useCallback( + async ({ + background = false, + supersede = false, + }: AsyncResourceReloadOptions = {}) => { + if (!enabled || !scopeKey) return; + if (supersede) coordinator.supersede(); + + setState((current) => { + const isCurrentScope = current.scopeKey === scopeKey; + const data = isCurrentScope ? current.data : initialDataRef.current; + if (background && isCurrentScope && current.status === "ready") { + return { ...current, error: null }; + } + return { + data, + error: null, + scopeKey, + status: + isCurrentScope && current.status !== "idle" + ? "refreshing" + : "loading", + }; + }); + + await coordinator.run(scopeKey, async (context) => { + try { + const publish = (data: T, options?: { keepLoading?: boolean }) => { + if (context.isCurrent()) { + setState((current) => ({ + data, + error: null, + scopeKey, + status: options?.keepLoading ? current.status : "ready", + })); + } + }; + const cause = background + ? "background" + : supersede + ? "refresh" + : "load"; + const data = await fetcher(scopeKey, { + cause, + isCurrent: context.isCurrent, + publish, + }); + if (context.isCurrent()) { + setState({ + data, + error: null, + scopeKey, + status: "ready", + }); + } + } catch (error) { + if (context.isCurrent()) { + setState((current) => ({ + data: + current.scopeKey === scopeKey + ? current.data + : initialDataRef.current, + error: error instanceof Error ? error.message : String(error), + scopeKey, + status: "error", + })); + } + } + }); + }, + [coordinator, enabled, fetcher, scopeKey] + ); + + useEffect(() => { + coordinator.supersede(); + if (!enabled || !scopeKey) { + setState({ + data: initialDataRef.current, + error: null, + scopeKey: null, + status: initialStatusRef.current, + }); + return undefined; + } + + if (autoLoad) { + void reload(); + } else { + setState({ + data: initialDataRef.current, + error: null, + scopeKey, + status: initialStatusRef.current, + }); + } + + return () => { + coordinator.supersede(); + }; + }, [autoLoad, coordinator, enabled, reload, scopeKey]); + + const visibleState = + enabled && scopeKey && state.scopeKey === scopeKey + ? state + : { + data: initialDataRef.current, + error: null, + scopeKey: null, + status: initialStatusRef.current, + }; + + const setData = useCallback>>( + (next) => { + if (!enabled || !scopeKey) return; + setState((current) => { + const currentData = + current.scopeKey === scopeKey ? current.data : initialDataRef.current; + return { + ...current, + data: + typeof next === "function" + ? (next as (current: T) => T)(currentData) + : next, + scopeKey, + }; + }); + }, + [enabled, scopeKey] + ); + + const refresh = useCallback(() => reload({ supersede: true }), [reload]); + + return { + data: visibleState.data, + error: visibleState.error, + loading: + visibleState.status === "loading" || visibleState.status === "refreshing", + refreshing: visibleState.status === "refreshing", + reload, + refresh, + setData, + status: visibleState.status, + }; +} diff --git a/src/hooks/async/useVisibilityPolledData.test.ts b/src/hooks/async/useVisibilityPolledData.test.ts new file mode 100644 index 0000000000..acac91c1c0 --- /dev/null +++ b/src/hooks/async/useVisibilityPolledData.test.ts @@ -0,0 +1,203 @@ +// @vitest-environment jsdom +import { act, createElement, useEffect } from "react"; +import { type Root, createRoot } from "react-dom/client"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { + type UseVisibilityPolledDataResult, + useVisibilityPolledData, +} from "./useVisibilityPolledData"; + +const pollMocks = vi.hoisted(() => ({ + start: vi.fn(), +})); + +vi.mock("@src/util/core/visibilityAwarePoll", () => ({ + startVisibilityAwarePoll: pollMocks.start, +})); + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, reject, resolve }; +} + +async function flush(): Promise { + await act(async () => { + await Promise.resolve(); + }); +} + +describe("useVisibilityPolledData", () => { + let container: HTMLDivElement; + let root: Root; + let current: UseVisibilityPolledDataResult; + let pollTasks: Array<() => Promise | void>; + const actEnvironment = globalThis as typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean; + }; + + function Harness({ + enabled = true, + fetcher, + scopeKey, + }: { + enabled?: boolean; + fetcher: (scopeKey: string) => Promise; + scopeKey: string | null; + }) { + const result = useVisibilityPolledData({ + enabled, + fetcher, + initialData: "", + intervalMs: 1_500, + scopeKey, + }); + useEffect(() => { + current = result; + }, [result]); + return createElement("div", { + "data-error": result.error ?? "", + "data-loading": String(result.loading), + "data-value": result.data, + }); + } + + beforeAll(() => { + actEnvironment.IS_REACT_ACT_ENVIRONMENT = true; + }); + + beforeEach(() => { + pollTasks = []; + pollMocks.start.mockReset().mockImplementation((options) => { + pollTasks.push(options.task); + if (options.runImmediately) void options.task(); + return { runNow: vi.fn(), stop: vi.fn() }; + }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + afterAll(() => { + Reflect.deleteProperty(actEnvironment, "IS_REACT_ACT_ENVIRONMENT"); + }); + + it("loads once, then refreshes in the background without clearing data", async () => { + const first = deferred(); + const second = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + expect(container.firstElementChild?.getAttribute("data-loading")).toBe( + "true" + ); + + first.resolve("first"); + await flush(); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "first" + ); + expect(container.firstElementChild?.getAttribute("data-loading")).toBe( + "false" + ); + + let background!: Promise; + act(() => { + background = Promise.resolve(pollTasks.at(-1)?.()); + }); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "first" + ); + expect(container.firstElementChild?.getAttribute("data-loading")).toBe( + "false" + ); + + second.resolve("second"); + await act(async () => background); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "second" + ); + }); + + it("drops a late response after switching scope", async () => { + const first = deferred(); + const second = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockImplementation((scope) => + scope === "a" ? first.promise : second.promise + ); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "b" }))); + + second.resolve("new scope"); + await flush(); + first.resolve("old scope"); + await flush(); + + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "new scope" + ); + }); + + it("recovers from failure through manual refresh", async () => { + const failed = deferred(); + const fetcher = vi + .fn<(scopeKey: string) => Promise>() + .mockReturnValueOnce(failed.promise) + .mockResolvedValueOnce("recovered"); + + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + failed.reject(new Error("offline")); + await flush(); + expect(container.firstElementChild?.getAttribute("data-error")).toBe( + "offline" + ); + + await act(async () => current.refresh()); + expect(container.firstElementChild?.getAttribute("data-value")).toBe( + "recovered" + ); + expect(container.firstElementChild?.getAttribute("data-error")).toBe(""); + }); + + it("clears scoped data and stops polling when disabled", async () => { + const fetcher = vi.fn().mockResolvedValue("loaded"); + act(() => root.render(createElement(Harness, { fetcher, scopeKey: "a" }))); + await flush(); + + act(() => + root.render( + createElement(Harness, { enabled: false, fetcher, scopeKey: "a" }) + ) + ); + + expect(container.firstElementChild?.getAttribute("data-value")).toBe(""); + expect(container.firstElementChild?.getAttribute("data-loading")).toBe( + "false" + ); + }); +}); diff --git a/src/hooks/async/useVisibilityPolledData.ts b/src/hooks/async/useVisibilityPolledData.ts new file mode 100644 index 0000000000..929f7bc3da --- /dev/null +++ b/src/hooks/async/useVisibilityPolledData.ts @@ -0,0 +1,68 @@ +import { useEffect } from "react"; + +import { startVisibilityAwarePoll } from "@src/util/core/visibilityAwarePoll"; + +import { useAsyncResource } from "./useAsyncResource"; + +export interface UseVisibilityPolledDataOptions { + enabled: boolean; + fetcher: (scopeKey: string) => Promise; + initialData: T; + intervalMs: number; + scopeKey: string | null; +} + +export interface UseVisibilityPolledDataResult { + data: T; + error: string | null; + loading: boolean; + refresh: () => Promise; +} + +/** + * Own one visibility-aware, scope-fenced polling resource. + * + * The first load and manual refresh expose loading state. Background ticks + * retain the current data without flashing the loading indicator. + */ +export function useVisibilityPolledData({ + enabled, + fetcher, + initialData, + intervalMs, + scopeKey, +}: UseVisibilityPolledDataOptions): UseVisibilityPolledDataResult { + const resource = useAsyncResource({ + autoLoad: false, + enabled, + fetcher, + initialData, + scopeKey, + }); + const { reload } = resource; + + useEffect(() => { + if (!enabled || !scopeKey) return undefined; + + let initialLoad = true; + const poll = startVisibilityAwarePoll({ + intervalMs, + runImmediately: true, + task: () => { + const background = !initialLoad; + initialLoad = false; + return reload({ background }); + }, + }); + return () => { + poll.stop(); + }; + }, [enabled, intervalMs, reload, scopeKey]); + + return { + data: resource.data, + error: resource.error, + loading: resource.loading, + refresh: resource.refresh, + }; +} 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 6ad5247414..85a05225d0 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/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/core/__tests__/latestScopedTask.test.ts b/src/util/core/__tests__/latestScopedTask.test.ts new file mode 100644 index 0000000000..022f8cb38c --- /dev/null +++ b/src/util/core/__tests__/latestScopedTask.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it, vi } from "vitest"; + +import { LatestScopedTask } from "../latestScopedTask"; + +describe("LatestScopedTask", () => { + it("shares one promise for the same scope", async () => { + const coordinator = new LatestScopedTask(); + const operation = vi.fn().mockResolvedValue("done"); + + const first = coordinator.run("same", operation); + const second = coordinator.run("same", operation); + + expect(first).toBe(second); + await expect(first).resolves.toBe("done"); + expect(operation).toHaveBeenCalledTimes(1); + }); + + it("marks an older scope stale as soon as a newer scope starts", async () => { + const coordinator = new LatestScopedTask(); + let oldIsCurrent!: () => boolean; + let release!: () => void; + const old = coordinator.run( + "old", + (context) => + new Promise((resolve) => { + oldIsCurrent = context.isCurrent; + release = resolve; + }) + ); + + await coordinator.run("new", async (context) => { + expect(context.isCurrent()).toBe(true); + }); + expect(oldIsCurrent()).toBe(false); + release(); + await old; + }); + + it("retries a scope after failure", async () => { + const coordinator = new LatestScopedTask(); + const operation = vi + .fn() + .mockRejectedValueOnce(new Error("failed")) + .mockResolvedValueOnce("retried"); + + await expect(coordinator.run("scope", operation)).rejects.toThrow("failed"); + await expect(coordinator.run("scope", operation)).resolves.toBe("retried"); + expect(operation).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/util/core/__tests__/visibilityAwarePoll.test.ts b/src/util/core/__tests__/visibilityAwarePoll.test.ts new file mode 100644 index 0000000000..3e16f5cdc2 --- /dev/null +++ b/src/util/core/__tests__/visibilityAwarePoll.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it, vi } from "vitest"; + +import { + type PollEnvironment, + startVisibilityAwarePoll, +} from "../visibilityAwarePoll"; + +function createEnvironment() { + let visible = true; + let visibilityListener: (() => void) | undefined; + const environment: PollEnvironment = { + clearTimer: (timer) => clearTimeout(timer as ReturnType), + isVisible: () => visible, + scheduleTimer: (callback, delayMs) => setTimeout(callback, delayMs), + subscribeToVisibilityChange: (callback) => { + visibilityListener = callback; + return () => { + visibilityListener = undefined; + }; + }, + }; + return { + environment, + setVisible(next: boolean) { + visible = next; + visibilityListener?.(); + }, + }; +} + +describe("startVisibilityAwarePoll", () => { + it("waits for the active task before scheduling another pass", async () => { + vi.useFakeTimers(); + const { environment } = createEnvironment(); + let release!: () => void; + const task = vi.fn( + () => + new Promise((resolve) => { + release = resolve; + }) + ); + const poll = startVisibilityAwarePoll({ + environment, + intervalMs: 2_000, + task, + }); + + await vi.advanceTimersByTimeAsync(2_000); + expect(task).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(10_000); + expect(task).toHaveBeenCalledTimes(1); + + release(); + await vi.runAllTicks(); + await vi.advanceTimersByTimeAsync(2_000); + expect(task).toHaveBeenCalledTimes(2); + + poll.stop(); + vi.useRealTimers(); + }); + + it("drops its timer while hidden and catches up once on visibility", async () => { + vi.useFakeTimers(); + const controlled = createEnvironment(); + const task = vi.fn().mockResolvedValue(undefined); + const poll = startVisibilityAwarePoll({ + environment: controlled.environment, + intervalMs: 2_000, + task, + }); + + controlled.setVisible(false); + await vi.advanceTimersByTimeAsync(10_000); + expect(task).not.toHaveBeenCalled(); + + controlled.setVisible(true); + await vi.runAllTicks(); + expect(task).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(2_000); + expect(task).toHaveBeenCalledTimes(2); + + poll.stop(); + vi.useRealTimers(); + }); + + it("does not reschedule after stop while a task is settling", async () => { + vi.useFakeTimers(); + const { environment } = createEnvironment(); + let release!: () => void; + const task = vi.fn( + () => + new Promise((resolve) => { + release = resolve; + }) + ); + const poll = startVisibilityAwarePoll({ + environment, + intervalMs: 2_000, + runImmediately: true, + task, + }); + + expect(task).toHaveBeenCalledTimes(1); + poll.stop(); + release(); + await vi.runAllTicks(); + await vi.advanceTimersByTimeAsync(10_000); + expect(task).toHaveBeenCalledTimes(1); + vi.useRealTimers(); + }); +}); diff --git a/src/util/core/latestScopedTask.ts b/src/util/core/latestScopedTask.ts new file mode 100644 index 0000000000..ddec2d3a53 --- /dev/null +++ b/src/util/core/latestScopedTask.ts @@ -0,0 +1,49 @@ +export interface ScopedTaskContext { + readonly generation: number; + isCurrent(): boolean; +} + +interface ActiveScopedTask { + key: string; + promise: Promise; +} + +/** + * Join equal async scopes while allowing a changed scope to supersede them. + * + * Callers use `context.isCurrent()` before committing results so late + * responses from a previous filter/project cannot overwrite newer state. + */ +export class LatestScopedTask { + private active?: ActiveScopedTask; + private generation = 0; + + run( + key: string, + operation: (context: ScopedTaskContext) => Promise + ): Promise { + if (this.active?.key === key) { + return this.active.promise as Promise; + } + + const generation = ++this.generation; + const context: ScopedTaskContext = { + generation, + isCurrent: () => this.generation === generation, + }; + const promise = operation(context); + this.active = { key, promise }; + const release = () => { + if (this.active?.promise === promise) { + this.active = undefined; + } + }; + void promise.then(release, release); + return promise; + } + + supersede(): void { + this.generation += 1; + this.active = undefined; + } +} diff --git a/src/util/core/visibilityAwarePoll.ts b/src/util/core/visibilityAwarePoll.ts new file mode 100644 index 0000000000..d50ca196db --- /dev/null +++ b/src/util/core/visibilityAwarePoll.ts @@ -0,0 +1,120 @@ +export interface PollEnvironment { + clearTimer(timer: unknown): void; + isVisible(): boolean; + scheduleTimer(callback: () => void, delayMs: number): unknown; + subscribeToVisibilityChange(callback: () => void): () => void; +} + +export interface VisibilityAwarePollOptions { + environment?: PollEnvironment; + intervalMs: number; + onError?: (error: unknown) => void; + runImmediately?: boolean; + runOnVisible?: boolean; + task: () => Promise | void; +} + +export interface VisibilityAwarePollController { + runNow(): void; + stop(): void; +} + +function browserPollEnvironment(): PollEnvironment { + return { + clearTimer: (timer) => window.clearTimeout(timer as number), + isVisible: () => document.visibilityState !== "hidden", + scheduleTimer: (callback, delayMs) => window.setTimeout(callback, delayMs), + subscribeToVisibilityChange: (callback) => { + document.addEventListener("visibilitychange", callback); + return () => document.removeEventListener("visibilitychange", callback); + }, + }; +} + +/** + * Run a non-critical background task without overlapping executions. + * + * The next delay starts only after the previous task settles. Hidden pages + * retain no timer; becoming visible triggers one immediate catch-up pass. + */ +export function startVisibilityAwarePoll( + options: VisibilityAwarePollOptions +): VisibilityAwarePollController { + const environment = options.environment ?? browserPollEnvironment(); + const runOnVisible = options.runOnVisible ?? true; + let stopped = false; + let running = false; + let rerunRequested = false; + let timer: unknown; + + const clearScheduledTimer = () => { + if (timer === undefined) return; + environment.clearTimer(timer); + timer = undefined; + }; + + const schedule = () => { + if (stopped || running || timer !== undefined || !environment.isVisible()) { + return; + } + timer = environment.scheduleTimer(() => { + timer = undefined; + void run(); + }, options.intervalMs); + }; + + const run = async () => { + if (stopped || !environment.isVisible()) return; + if (running) { + rerunRequested = true; + return; + } + + clearScheduledTimer(); + running = true; + try { + await options.task(); + } catch (error) { + options.onError?.(error); + } finally { + running = false; + if (!stopped) { + if (rerunRequested && environment.isVisible()) { + rerunRequested = false; + void run(); + } else { + rerunRequested = false; + schedule(); + } + } + } + }; + + const unsubscribe = environment.subscribeToVisibilityChange(() => { + if (!environment.isVisible()) { + clearScheduledTimer(); + return; + } + if (runOnVisible) { + void run(); + } else { + schedule(); + } + }); + + if (options.runImmediately) { + void run(); + } else { + schedule(); + } + + return { + runNow: () => void run(), + stop: () => { + stopped = true; + rerunRequested = false; + clearScheduledTimer(); + unsubscribe(); + }, + }; +} 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 // ============================================