diff --git a/apps/staged/src-tauri/Cargo.toml b/apps/staged/src-tauri/Cargo.toml index ce1432561..8852bd54f 100644 --- a/apps/staged/src-tauri/Cargo.toml +++ b/apps/staged/src-tauri/Cargo.toml @@ -85,6 +85,16 @@ tiny-skia = "0.12" tar = "0.4" flate2 = "1" +[dev-dependencies] +# test-util: `#[tokio::test(start_paused = true)]` for the store-events +# coalescer tests, so the 50ms window is driven by the paused clock instead +# of a real sleep. +tokio = { version = "1.50.0", features = ["test-util"] } +# test: tauri's `MockRuntime`, so window_commands' failure paths can be driven +# without a real event loop. Feature unification enables it only when +# compiling tests; the shipped binary's tauri is unchanged. +tauri = { version = "2.10.2", features = ["test"] } + [features] # no-block-npm-registry: downloads the managed Node.js runtime from upstream # nodejs.org instead of Block's Artifactory mirror, and lets npm-backed diff --git a/apps/staged/src-tauri/capabilities/default.json b/apps/staged/src-tauri/capabilities/default.json index 29e7d99b6..2f3e4acdb 100644 --- a/apps/staged/src-tauri/capabilities/default.json +++ b/apps/staged/src-tauri/capabilities/default.json @@ -2,7 +2,7 @@ "$schema": "../gen/schemas/desktop-schema.json", "identifier": "default", "description": "enables the default permissions", - "windows": ["main"], + "windows": ["main", "win-*"], "permissions": [ "core:default", "core:window:allow-start-dragging", @@ -13,6 +13,7 @@ "window-state:default", "store:default", "core:window:allow-set-badge-count", + "core:window:allow-set-title", "dialog:default", "process:allow-restart", "updater:default", diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 58a9b71fc..44968f501 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -36,10 +36,12 @@ pub mod session_completion; pub mod session_runner; pub mod shell_env; pub mod store; +pub mod store_events; pub(crate) mod terminal_output; pub mod timeline; pub mod util_commands; pub mod web_server; +pub mod window_commands; #[cfg(test)] pub mod test_utils; @@ -279,6 +281,15 @@ fn stop_actions_for_app_shutdown(app_handle: &tauri::AppHandle) { } } +fn start_store_services( + store: Arc, + pr_scheduler: Arc, + app_handle: tauri::AppHandle, +) { + background_sync::spawn(Arc::clone(&store), app_handle.clone()); + pr_poll_scheduler::spawn(pr_scheduler, store, app_handle); +} + // ============================================================================= // Store status commands // ============================================================================= @@ -292,16 +303,53 @@ fn get_store_status(db_state: tauri::State<'_, DbState>) -> Option>>, + store_change_tx: &tokio::sync::broadcast::Sender, +) -> Result>, String> { + // This guard is the reset claim. Holding it through file deletion, store + // creation, and slot replacement makes concurrent confirmations serialize; + // the loser observes `None` and must not delete the newly created store. + let mut needs_reset = db_state.needs_reset.lock().unwrap(); + match needs_reset.as_ref() { + None => return Ok(None), + Some(info) if info.kind == "needs_reset" => {} + Some(_) => { + return Err( + "Database was created by a newer Staged version and cannot be reset".to_string(), + ); + } + } + + store::remove_db_files(&db_state.db_path).map_err(|e| e.to_string())?; + + let store = Arc::new( + Store::new(&db_state.db_path) + .map_err(|e| e.to_string())? + .with_change_sender(store_change_tx.clone()), + ); + *store_slot.lock().unwrap() = Some(Arc::clone(&store)); + *needs_reset = None; + Ok(Some(store)) +} + #[tauri::command] fn confirm_reset_store( + app_handle: tauri::AppHandle, db_state: tauri::State<'_, DbState>, store_slot: tauri::State<'_, Mutex>>>, + store_change_tx: tauri::State<'_, tokio::sync::broadcast::Sender>, + pr_scheduler: tauri::State<'_, Arc>, ) -> Result<(), String> { - store::remove_db_files(&db_state.db_path).map_err(|e| e.to_string())?; - - let s = Store::new(&db_state.db_path).map_err(|e| e.to_string())?; - *store_slot.lock().unwrap() = Some(Arc::new(s)); - *db_state.needs_reset.lock().unwrap() = None; + if let Some(store) = reset_store(&db_state, &store_slot, &store_change_tx)? { + start_store_services(store, Arc::clone(pr_scheduler.inner()), app_handle.clone()); + // Every window owns its prompt state. Tell peers to dismiss it after + // the shared backend store has been replaced successfully. + if let Err(error) = app_handle.emit("store-reset-completed", ()) { + log::warn!("Failed to broadcast store reset completion: {error}"); + } + } Ok(()) } @@ -1717,6 +1765,53 @@ fn delete_action_context( // Tauri App Setup // ============================================================================= +/// What the app menu handler should do with a menu event. +#[derive(Debug, PartialEq, Eq)] +enum MenuDispatch { + /// Forward as this frontend event, addressed to the focused window. + EmitToFocused(&'static str), + /// Create a window here in the backend, with no project seed. + OpenWindowUnseeded, + /// Nothing to do — unknown item, or a window-scoped item with no target. + Drop, +} + +/// Route a menu item to its handler. Menu actions apply to the focused window +/// only — a broadcast would e.g. open settings in every window, or fire Delete +/// Project in each window against its own selected project. +/// +/// With no window focused (every window minimized — reachable on macOS, where +/// the app menu stays live) window-scoped items drop, like a disabled menu item: +/// routing them to an arbitrary minimized window would open settings invisibly, +/// or delete whichever project that window happened to have selected. New Window +/// is the exception. It's exactly what a user reaches for when nothing is +/// visible, and it only round-trips through the frontend to inherit the opener's +/// selected project — with no opener there is nothing to inherit, so the backend +/// can just create it. That also un-strands the other items: the new window is +/// focused, so Settings/Find/zoom route normally again. +fn dispatch_menu_event(id: &str, has_focused_window: bool) -> MenuDispatch { + let event_name = match id { + "new_window" => "menu:new-window", + "settings" => "menu:settings", + "find" => "menu:find", + "find_next" => "menu:find-next", + "find_previous" => "menu:find-previous", + "delete_project" => "menu:delete-project", + "zoom_in" => "menu:zoom-in", + "zoom_out" => "menu:zoom-out", + "zoom_reset" => "menu:zoom-reset", + _ => return MenuDispatch::Drop, + }; + + if has_focused_window { + MenuDispatch::EmitToFocused(event_name) + } else if id == "new_window" { + MenuDispatch::OpenWindowUnseeded + } else { + MenuDispatch::Drop + } +} + #[cfg_attr(mobile, tauri::mobile_entry_point)] pub fn run() { tauri::Builder::default() @@ -1733,6 +1828,12 @@ pub fn run() { tauri_plugin_window_state::StateFlags::all() & !tauri_plugin_window_state::StateFlags::VISIBLE, ) + // Only track the main window. Secondary `win-*` windows get + // fresh labels each launch, so persisting their geometry would + // accumulate stale entries in the state file that are never + // restored — they are placed by cascade instead (see + // `window_commands::new_window`). + .with_filter(|label| label == "main") .build(), ) .plugin(tauri_plugin_store::Builder::new().build()) @@ -1793,6 +1894,16 @@ pub fn run() { true, Some("CmdOrCtrl+,"), )?; + // ⇧⌘N, not plain ⌘N: the native accelerator consumes the + // keydown before the webview sees it, and ⌘N belongs to the + // frontend's New Project shortcut (`app-new-project`). + let new_window_item = MenuItem::with_id( + handle, + "new_window", + "New Window", + true, + Some("CmdOrCtrl+Shift+N"), + )?; let find_item = MenuItem::with_id(handle, "find", "Find…", true, Some("CmdOrCtrl+F"))?; let find_next_item = @@ -1849,7 +1960,11 @@ pub fn run() { handle, "File", true, - &[&PredefinedMenuItem::close_window(handle, None)?], + &[ + &new_window_item, + &PredefinedMenuItem::separator(handle)?, + &PredefinedMenuItem::close_window(handle, None)?, + ], )?; let edit_menu = Submenu::with_items( @@ -1925,16 +2040,26 @@ pub fn run() { let compat = store::check_db_compatibility(&db_path) .map_err(|e| format!("Cannot check database: {e}"))?; let session_registry = Arc::new(session_runner::SessionRegistry::new()); + // Store change feed: every mutating store method publishes a + // StoreChange here; the coalescer forwards them to all windows + // and web clients as domain events. Created unconditionally + // (like the scheduler) so `confirm_reset_store` can wire the + // same feed into a replacement store. + let (store_change_tx, store_change_rx) = + tokio::sync::broadcast::channel::(1024); + store_events::spawn(app.handle().clone(), store_change_rx); + app.manage(store_change_tx.clone()); // Backend-owned PR-poll scheduler. Managed unconditionally so the // interest/hint commands resolve even before the store exists (e.g. - // during the needs-reset prompt); the tick loop is only spawned once - // the store is ready (the `Ok` branch below). + // during the needs-reset prompt); the tick loop is spawned once the + // store is ready, either below or after a confirmed reset. let pr_scheduler = Arc::new(pr_poll_scheduler::PrPollScheduler::new()); let (store_slot, reset_info) = match compat { store::DbCompatibility::Ok => { - let s = - Store::new(&db_path).map_err(|e| format!("Failed to open store: {e}"))?; + let s = Store::new(&db_path) + .map_err(|e| format!("Failed to open store: {e}"))? + .with_change_sender(store_change_tx.clone()); let store_arc = Arc::new(s); // Recover sessions whose owner process is dead; leave sessions // owned by other live Staged instances untouched. @@ -1964,13 +2089,12 @@ pub fn run() { Ok(n) => log::info!("Cleaned up {n} pending image(s) from previous run"), Err(e) => log::warn!("Failed to clean up pending images: {e}"), } - // Start the tiered background sync service for all cloned repos. - background_sync::spawn(Arc::clone(&store_arc), app.handle().clone()); - // Start the backend PR-poll scheduler — it owns polling - // cadence/concurrency; the frontend only sends interest hints. - pr_poll_scheduler::spawn( - Arc::clone(&pr_scheduler), + // Start the store-backed services only once the store is + // ready. The reset path calls the same helper after it + // creates a compatible replacement. + start_store_services( Arc::clone(&store_arc), + Arc::clone(&pr_scheduler), app.handle().clone(), ); // `fsmonitor-v1` only flips `.git/config` flags on stale @@ -2019,6 +2143,8 @@ pub fn run() { app.manage(store_slot); app.manage(session_registry); app.manage(pr_scheduler); + app.manage(window_commands::NewWindowState::new()); + app.manage(window_commands::UpdaterWindowState::default()); app.manage(Arc::new(actions::ActionExecutor::new())); app.manage(Arc::new(actions::ActionRegistry::new())); app.manage(ShutdownState::default()); @@ -2060,27 +2186,66 @@ pub fn run() { Ok(()) }) .on_menu_event(|app, event| { - let maybe_event_name = match event.id().as_ref() { - "settings" => Some("menu:settings"), - "find" => Some("menu:find"), - "find_next" => Some("menu:find-next"), - "find_previous" => Some("menu:find-previous"), - "delete_project" => Some("menu:delete-project"), - "zoom_in" => Some("menu:zoom-in"), - "zoom_out" => Some("menu:zoom-out"), - "zoom_reset" => Some("menu:zoom-reset"), - _ => None, - }; - - if let Some(event_name) = maybe_event_name { - if let Err(e) = app.emit(event_name, ()) { - log::warn!("Failed to emit {event_name} event: {e}"); + // Thin interpreter over `dispatch_menu_event`, which owns the + // routing rules (and their tests). + let focused = window_commands::focused_window(app); + match dispatch_menu_event(event.id().as_ref(), focused.is_some()) { + MenuDispatch::EmitToFocused(event_name) => { + // Some by construction: EmitToFocused is only returned when + // `focused.is_some()`. + if let Some(window) = focused { + if let Err(e) = app.emit_to(window.label(), event_name, ()) { + log::warn!("Failed to emit {event_name} event: {e}"); + } + } + } + MenuDispatch::OpenWindowUnseeded => { + // Menus exist only on macOS, where menu events are delivered + // on the main thread — the same thread `setup` builds the + // first window on. + if let Err(e) = window_commands::open_new_window(app, None) { + log::warn!("Failed to open window from menu: {e}"); + } + } + MenuDispatch::Drop => {} + } + }) + .on_window_event(|window, event| { + if let tauri::WindowEvent::Destroyed = event { + // Native windows have no WS heartbeat and their PR-poll client + // ids are exempt from TTL eviction, so a closed window must + // explicitly drop its interest or the scheduler keeps polling + // at that window's cadence forever. + let app = window.app_handle(); + app.state::>() + .disconnect_client(format!( + "{}{}", + pr_poll_scheduler::TAURI_CLIENT_PREFIX, + window.label() + )); + // Drop any unconsumed navigation seed (window closed pre-init). + app.state::() + .discard_seed(window.label()); + // The updater UI is window-owned but process-wide. A native + // destruction hook is the reliable handoff point even when the + // webview's frontend teardown never runs. + if app + .state::() + .window_destroyed(window.label()) + { + if let Err(error) = app.emit("updater-owner-available", ()) { + log::warn!("Failed to announce updater ownership release: {error}"); + } } } }) .invoke_handler(tauri::generate_handler![ get_store_status, confirm_reset_store, + // Windows + window_commands::new_window, + window_commands::take_window_seed, + window_commands::claim_updater_ownership, list_projects, create_project, list_project_repos, @@ -2292,9 +2457,141 @@ pub fn run() { #[cfg(test)] mod tests { - use super::cleanup_project_branches_best_effort; + use super::{ + cleanup_project_branches_best_effort, dispatch_menu_event, reset_store, DbState, + MenuDispatch, StoreIncompatibility, + }; use crate::store::{Branch, BranchType}; use std::collections::HashMap; + use std::sync::{Arc, Barrier, Mutex}; + + fn reset_info(kind: &str) -> StoreIncompatibility { + StoreIncompatibility { + db_app_version: "0.1.0".to_string(), + app_version: "0.2.0".to_string(), + kind: kind.to_string(), + } + } + + #[test] + fn concurrent_store_resets_create_the_replacement_once() { + let dir = tempfile::tempdir().unwrap(); + let db_state = Arc::new(DbState { + db_path: dir.path().join("data.db"), + needs_reset: Mutex::new(Some(reset_info("needs_reset"))), + }); + let store_slot = Arc::new(Mutex::new(None)); + let (store_change_tx, _) = tokio::sync::broadcast::channel(4); + let barrier = Arc::new(Barrier::new(3)); + + let handles: Vec<_> = (0..2) + .map(|_| { + let db_state = Arc::clone(&db_state); + let store_slot = Arc::clone(&store_slot); + let store_change_tx = store_change_tx.clone(); + let barrier = Arc::clone(&barrier); + std::thread::spawn(move || { + barrier.wait(); + reset_store(&db_state, &store_slot, &store_change_tx) + .unwrap() + .is_some() + }) + }) + .collect(); + + barrier.wait(); + let results: Vec<_> = handles + .into_iter() + .map(|handle| handle.join().unwrap()) + .collect(); + + assert_eq!(results.iter().filter(|performed| **performed).count(), 1); + assert!(store_slot.lock().unwrap().is_some()); + assert!(db_state.needs_reset.lock().unwrap().is_none()); + assert!(db_state.db_path.exists()); + } + + #[test] + fn a_too_new_store_cannot_be_reset_by_invoking_the_command_directly() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("data.db"); + std::fs::write(&db_path, b"newer database").unwrap(); + let db_state = DbState { + db_path: db_path.clone(), + needs_reset: Mutex::new(Some(reset_info("too_new"))), + }; + let store_slot = Mutex::new(None); + let (store_change_tx, _) = tokio::sync::broadcast::channel(4); + + let error = match reset_store(&db_state, &store_slot, &store_change_tx) { + Err(error) => error, + Ok(_) => panic!("too-new store should not be reset"), + }; + + assert!(error.contains("newer Staged version")); + assert_eq!(std::fs::read(db_path).unwrap(), b"newer database"); + assert!(store_slot.lock().unwrap().is_none()); + assert_eq!( + db_state + .needs_reset + .lock() + .unwrap() + .as_ref() + .map(|info| info.kind.as_str()), + Some("too_new") + ); + } + + /// Every menu item this app defines, with the frontend event it routes to. + const MENU_ITEMS: &[(&str, &str)] = &[ + ("new_window", "menu:new-window"), + ("settings", "menu:settings"), + ("find", "menu:find"), + ("find_next", "menu:find-next"), + ("find_previous", "menu:find-previous"), + ("delete_project", "menu:delete-project"), + ("zoom_in", "menu:zoom-in"), + ("zoom_out", "menu:zoom-out"), + ("zoom_reset", "menu:zoom-reset"), + ]; + + #[test] + fn menu_events_go_to_the_focused_window() { + for (id, event_name) in MENU_ITEMS { + assert_eq!( + dispatch_menu_event(id, true), + MenuDispatch::EmitToFocused(event_name), + "menu item {id} should emit {event_name} to the focused window" + ); + } + } + + #[test] + fn new_window_falls_back_to_native_creation_with_no_focused_window() { + assert_eq!( + dispatch_menu_event("new_window", false), + MenuDispatch::OpenWindowUnseeded + ); + } + + #[test] + fn other_menu_events_drop_with_no_focused_window() { + for (id, _) in MENU_ITEMS.iter().filter(|(id, _)| *id != "new_window") { + assert_eq!( + dispatch_menu_event(id, false), + MenuDispatch::Drop, + "window-scoped menu item {id} has no target and should drop" + ); + } + } + + #[test] + fn unknown_menu_events_drop_regardless_of_focus() { + for id in ["", "quit", "menu:new-window", "New Window"] { + assert_eq!(dispatch_menu_event(id, true), MenuDispatch::Drop); + assert_eq!(dispatch_menu_event(id, false), MenuDispatch::Drop); + } + } fn remote_branch( project_id: &str, diff --git a/apps/staged/src-tauri/src/pr_poll_scheduler.rs b/apps/staged/src-tauri/src/pr_poll_scheduler.rs index e7ff97732..893e8247f 100644 --- a/apps/staged/src-tauri/src/pr_poll_scheduler.rs +++ b/apps/staged/src-tauri/src/pr_poll_scheduler.rs @@ -14,7 +14,7 @@ //! //! ## Per-client interest (Phase 2) //! -//! Interest is tracked **per connected client** — the native Tauri window plus +//! Interest is tracked **per connected client** — each native Tauri window plus //! each WebSocket browser session — keyed by a frontend-supplied `client_id`. //! The cadence for a project is the union across all clients ([`PollState::any_focused`], //! [`PollState::is_foreground`], [`PollState::project_has_pending`]), so a project @@ -22,11 +22,20 @@ //! bookkeeping (`last_polled_at`/`failures`/`stale`/`forced`) stays project-keyed //! and shared, so N clients still trigger only one poll per project per tier. //! -//! Clients are evicted on disconnect (clean WS close ⇒ [`PrPollScheduler::disconnect_client`]) -//! and via a [`CLIENT_TTL_MS`] fallback for dirty drops ([`PollState::evict_stale_clients`], -//! swept each tick). The native window uses the fixed [`TAURI_CLIENT_ID`], which -//! is pre-seeded at launch and exempt from TTL eviction (process death is its -//! teardown), so single-client behaviour stays byte-for-byte equivalent to Phase 1. +//! Clients are evicted on disconnect (clean WS close or native window destroyed +//! ⇒ [`PrPollScheduler::disconnect_client`]) and via a [`CLIENT_TTL_MS`] fallback +//! for dirty drops ([`PollState::evict_stale_clients`], swept each tick). Native +//! windows use `tauri-{window label}` ids ([`TAURI_CLIENT_PREFIX`]), which are +//! exempt from TTL eviction — they have no WS heartbeat; the first window's id +//! ([`TAURI_CLIENT_ID`]) is pre-seeded at launch, so single-window behaviour +//! stays byte-for-byte equivalent to Phase 1. +//! +//! The TTL exemption is only sound because the `tauri-*` namespace is +//! *reserved*: [`is_reserved_client_id`] names the invariant, and the web +//! boundaries in `web_server.rs` (the `/api/events` WS `clientId` and the +//! PR-poll `/api/dispatch` verbs) reject ids that claim it. An exempt entry +//! must have a window-`Destroyed` teardown behind it, so the exemption and the +//! rejection are one invariant split across two files. //! //! Poll-state (last-polled timestamps, failure counts) is intentionally **not //! persisted** — on restart everything is "due", matching the frontend's @@ -68,9 +77,15 @@ const MAX_CONSECUTIVE_FAILURES: u32 = 3; /// wake the loop immediately, so this only bounds the *periodic* re-poll delay. const TICK_INTERVAL_SECS: u64 = 5; -/// Well-known id for the native Tauri window. It has no WS heartbeat (the -/// process dying is its teardown), so it is pre-seeded at launch and exempt from -/// TTL eviction. Must match `TAURI_CLIENT_ID` in `prPollingService.ts`. +/// Id prefix for native Tauri windows: `tauri-{window label}`. Native windows +/// have no WS heartbeat, so ids with this prefix are exempt from TTL eviction — +/// their teardown is the window being destroyed (the `on_window_event` hook in +/// `lib.rs` calls [`PrPollScheduler::disconnect_client`]) or the process dying. +/// Must match the prefix used in `prPollingService.ts`. +pub const TAURI_CLIENT_PREFIX: &str = "tauri-"; + +/// Well-known id for the first native window (label `main`). Pre-seeded at +/// launch as focused so the very first tick polls before any hint arrives. const TAURI_CLIENT_ID: &str = "tauri-main"; /// How long a client's interest survives without a heartbeat before the tick @@ -80,6 +95,20 @@ const TAURI_CLIENT_ID: &str = "tauri-main"; /// counted client to ≲6. The Tauri id is exempt. const CLIENT_TTL_MS: i64 = 90_000; +/// Whether a caller-supplied client id claims the native-window namespace. +/// +/// `tauri-*` ids are exempt from TTL eviction ([`PollState::evict_stale_clients`]), +/// which is only safe when the id was minted by native window code — teardown is +/// then guaranteed by the window-`Destroyed` hook in `lib.rs`. Web boundaries (the +/// `/api/events` WS `clientId`, the PR-poll `/api/dispatch` verbs) must reject +/// these: a web client claiming one would leak its interest forever on a dirty +/// drop (nothing evicts it, and no window exists to be destroyed), or spoof a real +/// window's entry. Legitimate web clients use a UUID, so rejecting the namespace +/// can never hit one. +pub fn is_reserved_client_id(id: &str) -> bool { + id.starts_with(TAURI_CLIENT_PREFIX) +} + // --------------------------------------------------------------------------- // Poll-state — pure decision logic, no clock / store / Tauri handles // --------------------------------------------------------------------------- @@ -310,12 +339,14 @@ impl PollState { self.clients.remove(client_id); } - /// Dirty-drop fallback: evict clients not heard from within `ttl_ms`. The - /// Tauri id is exempt (the native window has no WS heartbeat; the process - /// dying tears it down). + /// Dirty-drop fallback: evict clients not heard from within `ttl_ms`. + /// Native window ids ([`is_reserved_client_id`]) are exempt — they have no WS + /// heartbeat; window destruction or process death tears them down. That + /// guarantee holds only because the web boundaries reject the reserved + /// namespace, so an exempt id here is always a real window's. fn evict_stale_clients(&mut self, now: i64, ttl_ms: i64) { self.clients - .retain(|id, c| id == TAURI_CLIENT_ID || now.saturating_sub(c.last_seen) <= ttl_ms); + .retain(|id, c| is_reserved_client_id(id) || now.saturating_sub(c.last_seen) <= ttl_ms); } } @@ -959,6 +990,8 @@ mod tests { st.set_focus("web", true, 0); st.set_foreground("web", Some("p".into()), 0); assert!(st.is_foreground("p")); + // A second native window: no heartbeat, idle since launch. + st.set_foreground("tauri-win-2", Some("q".into()), 0); // Sweep well past the TTL relative to last_seen = 0. st.evict_stale_clients(CLIENT_TTL_MS + 1, CLIENT_TTL_MS); @@ -966,9 +999,30 @@ mod tests { // The stale web client is gone; its interest no longer counts. assert!(!st.clients.contains_key("web")); assert!(!st.is_foreground("p")); - // The Tauri client is exempt despite last_seen = 0, and stays focused. + // Native window ids are exempt despite last_seen = 0: the first window + // stays focused and the idle second window keeps its foreground. assert!(st.clients.contains_key(TAURI_CLIENT_ID)); assert!(st.any_focused()); + assert!(st.is_foreground("q")); + + // A destroyed native window is torn down via explicit disconnect. + st.disconnect_client("tauri-win-2"); + assert!(!st.is_foreground("q")); + } + + #[test] + fn reserved_client_ids_are_the_tauri_namespace() { + // Exactly the ids the native windows mint (see `prPollingService.ts`). + assert!(is_reserved_client_id(TAURI_CLIENT_ID)); + assert!(is_reserved_client_id("tauri-win-2")); + // Web ids never claim the namespace; the match is an exact, case- + // sensitive prefix, matching the frontend's lowercase minting. + assert!(!is_reserved_client_id("3f1a-uuid")); + assert!(!is_reserved_client_id("")); + assert!(!is_reserved_client_id("TAURI-main")); + assert!(!is_reserved_client_id("tauri")); + assert!(!is_reserved_client_id(" tauri-main")); + assert!(!is_reserved_client_id("web-tauri-main")); } #[test] diff --git a/apps/staged/src-tauri/src/store/branch_move.rs b/apps/staged/src-tauri/src/store/branch_move.rs index 838c6a3bd..246d82561 100644 --- a/apps/staged/src-tauri/src/store/branch_move.rs +++ b/apps/staged/src-tauri/src/store/branch_move.rs @@ -12,7 +12,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::ProjectRepo; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; /// Which `project_repos` row the moved branch points at once it lands. /// @@ -153,6 +153,26 @@ impl Store { elect_primary_repo(&tx, &mv.target_project_id, now)?; tx.commit()?; + // A move mutates *two* projects' branch surfaces, and `project_id` on a + // `Branch` change means "this project's branch list is affected", not + // "this branch's current parent" — so name both. Without the source + // publish nothing in the feed says the branch left, and a consumer that + // scopes its invalidation to the named project would leave the branch + // listed under its old project too. + self.publish(StoreChange::Branch { + branch_id: mv.branch_id.clone(), + project_id: Some(mv.target_project_id.clone()), + }); + self.publish(StoreChange::Branch { + branch_id: mv.branch_id.clone(), + project_id: Some(mv.source_project_id.clone()), + }); + self.publish(StoreChange::Project { + project_id: Some(mv.source_project_id.clone()), + }); + self.publish(StoreChange::Project { + project_id: Some(mv.target_project_id.clone()), + }); Ok(()) } } @@ -294,7 +314,12 @@ mod tests { /// A branch on its own `project_repos` row in `source`, plus an empty /// `target` project to move it into. fn fixture() -> Fixture { - let store = Store::in_memory().unwrap(); + fixture_in(Store::in_memory().unwrap()) + } + + /// [`fixture`] over a caller-supplied store, so the change-feed test can + /// build the same shape on a store with a sender attached. + fn fixture_in(store: Store) -> Fixture { let source = Project::named("source").with_primary_repo("acme/widgets"); let target = Project::named("target"); store.create_project(&source).unwrap(); @@ -327,6 +352,40 @@ mod tests { } } + /// A move rewrites two projects' branch lists, so the feed names both. A + /// consumer that scopes its invalidation to the named project has no other + /// way to hear that the branch left the source — the branch row itself now + /// points at the target, so an enrichment lookup could never resolve it. + #[test] + fn change_feed_publishes_one_branch_change_per_touched_project() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let f = fixture_in(Store::in_memory().unwrap().with_change_sender(tx)); + while rx.try_recv().is_ok() {} + + f.store.move_branch_to_project(&reparent(&f, None)).unwrap(); + + let branch_change = |project_id: &str| StoreChange::Branch { + branch_id: f.branch.id.clone(), + project_id: Some(project_id.to_string()), + }; + assert_eq!(rx.try_recv().unwrap(), branch_change(&f.target.id)); + assert_eq!(rx.try_recv().unwrap(), branch_change(&f.source.id)); + // The two `Project` changes cover the repo re-election on either side. + assert_eq!( + rx.try_recv().unwrap(), + StoreChange::Project { + project_id: Some(f.source.id.clone()), + } + ); + assert_eq!( + rx.try_recv().unwrap(), + StoreChange::Project { + project_id: Some(f.target.id.clone()), + } + ); + assert!(rx.try_recv().is_err()); + } + #[test] fn carries_the_branch_its_repo_row_and_its_images() { let f = fixture(); diff --git a/apps/staged/src-tauri/src/store/branches.rs b/apps/staged/src-tauri/src/store/branches.rs index 00d7e8fe0..a6b40c6a1 100644 --- a/apps/staged/src-tauri/src/store/branches.rs +++ b/apps/staged/src-tauri/src/store/branches.rs @@ -3,7 +3,22 @@ use rusqlite::{params, Connection, OptionalExtension}; use super::models::{Branch, BranchType, WorkspaceStatus}; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; + +/// The PR fields that carry domain state — everything +/// [`Store::update_branch_pr_status`] writes except the `pr_fetched_at` / +/// `updated_at` timestamps, which advance on every poll. +#[derive(PartialEq)] +struct PrStatusFields { + state: Option, + checks_status: Option, + review_decision: Option, + mergeable: Option, + draft: Option, + url: Option, + updated_at: Option, + head_sha: Option, +} impl Store { pub fn create_branch(&self, branch: &Branch) -> Result<(), StoreError> { @@ -51,6 +66,10 @@ impl Store { } return Err(e.into()); } + self.publish(StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(branch.project_id.clone()), + }); Ok(()) } @@ -95,6 +114,10 @@ impl Store { "UPDATE branches SET base_branch = ?1, updated_at = ?2 WHERE id = ?3", params![base_branch, now_timestamp(), id], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); Ok(()) } @@ -104,25 +127,50 @@ impl Store { "UPDATE branches SET branch_name = ?1, updated_at = ?2 WHERE id = ?3", params![branch_name, now_timestamp(), id], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); Ok(()) } /// Update the workspace status for a remote branch. + /// + /// The Blox poller calls this for every active workspace on every cycle, so + /// the common case is rewriting the value already stored. The `IS NOT` guard + /// makes the statement its own change detector: an unchanged status matches + /// no row, so nothing is written and nothing is published. `rows > 0` + /// therefore means "the status actually moved" — the same shape as + /// [`Store::mark_branch_setup_complete`], and the same contract as the + /// compare-before-publish in [`Store::update_branch_pr_status`]. (Unlike + /// that method the write itself is skippable: the only column a no-op would + /// touch is `updated_at`, which nothing reads.) pub fn update_branch_workspace_status( &self, id: &str, status: &WorkspaceStatus, ) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); - conn.execute( - "UPDATE branches SET workspace_status = ?1, updated_at = ?2 WHERE id = ?3", + let rows = conn.execute( + "UPDATE branches SET workspace_status = ?1, updated_at = ?2 + WHERE id = ?3 AND workspace_status IS NOT ?1", params![status.as_str(), now_timestamp(), id], )?; + if rows > 0 { + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); + } Ok(()) } /// Update workspace status for all branches sharing a given workspace name. - /// Returns the IDs of all updated branches. + /// Returns the IDs of all branches on the workspace, moved or not — both + /// callers read an empty vec as "no such workspace" and surface an error, so + /// resuming a workspace whose branches already hold `status` must not come + /// back empty. Only the branches whose status actually moved publish, for + /// the reason spelled out on [`Store::update_branch_workspace_status`]. pub fn update_workspace_status_by_workspace_name( &self, workspace_name: &str, @@ -130,15 +178,30 @@ impl Store { ) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); let now = now_timestamp(); + // Snapshotted before the write; the set is identical either side of it + // (the connection lock is held throughout and the UPDATE touches + // neither `workspace_name` nor row existence). + let mut stmt = + conn.prepare("SELECT id, workspace_status FROM branches WHERE workspace_name = ?1")?; + let previous: Vec<(String, Option)> = stmt + .query_map(params![workspace_name], |row| { + Ok((row.get(0)?, row.get(1)?)) + })? + .collect::, _>>()?; conn.execute( - "UPDATE branches SET workspace_status = ?1, updated_at = ?2 WHERE workspace_name = ?3", + "UPDATE branches SET workspace_status = ?1, updated_at = ?2 + WHERE workspace_name = ?3 AND workspace_status IS NOT ?1", params![status.as_str(), now, workspace_name], )?; - let mut stmt = conn.prepare("SELECT id FROM branches WHERE workspace_name = ?1")?; - let ids = stmt - .query_map(params![workspace_name], |row| row.get::<_, String>(0))? - .collect::, _>>()?; - Ok(ids) + for (id, prev) in &previous { + if prev.as_deref() != Some(status.as_str()) { + self.publish_with(|| StoreChange::Branch { + branch_id: id.clone(), + project_id: Self::branch_project_id(&conn, id), + }); + } + } + Ok(previous.into_iter().map(|(id, _)| id).collect()) } /// Update the PR number for a branch. @@ -152,10 +215,22 @@ impl Store { "UPDATE branches SET pr_number = ?1, updated_at = ?2 WHERE id = ?3", params![pr_number.map(|n| n as i64), now_timestamp(), id], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); Ok(()) } /// Update PR status fields for a branch. + /// + /// The PR poll scheduler calls this after *every* `gh` fetch, so the write + /// itself is unconditional — `pr_fetched_at` and `updated_at` always + /// advance — but the change feed only speaks up when the eight domain + /// fields actually move. A timestamp-only refresh is deliberately silent: + /// publishing it would drop every window's timeline and diff caches for + /// every branch with a PR, once per poll cycle. Freshness reaches the UI + /// through the cheap in-place `pr-status-changed` event instead. #[allow(clippy::too_many_arguments)] pub fn update_branch_pr_status( &self, @@ -171,7 +246,22 @@ impl Store { ) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); let now = now_timestamp(); - conn.execute( + let incoming = PrStatusFields { + state: pr_state, + checks_status: pr_checks_status, + review_decision: pr_review_decision, + mergeable: pr_mergeable, + draft: pr_draft, + url: pr_url, + updated_at: pr_updated_at, + head_sha: pr_head_sha, + }; + // Snapshot the domain fields before the write. `None` means the row is + // gone or unreadable, which compares as "changed" — the `rows > 0` + // guard below is what suppresses the publish for a missing branch, so + // a failed read never swallows a real change. + let previous = Self::read_pr_status_fields(&conn, id); + let rows = conn.execute( "UPDATE branches SET pr_state = ?1, pr_checks_status = ?2, @@ -185,22 +275,53 @@ impl Store { updated_at = ?10 WHERE id = ?11", params![ - pr_state, - pr_checks_status, - pr_review_decision, - pr_mergeable.map(|b| if b { 1 } else { 0 }), - pr_draft.map(|b| if b { 1 } else { 0 }), - pr_url, - pr_updated_at, + incoming.state, + incoming.checks_status, + incoming.review_decision, + incoming.mergeable.map(|b| if b { 1 } else { 0 }), + incoming.draft.map(|b| if b { 1 } else { 0 }), + incoming.url, + incoming.updated_at, now, - pr_head_sha, + incoming.head_sha, now, id ], )?; + if rows > 0 && previous.as_ref() != Some(&incoming) { + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); + } Ok(()) } + /// Read the PR fields that carry domain state, for the + /// compare-before-publish in [`Store::update_branch_pr_status`]. + /// `None` on a missing row or a read error. + fn read_pr_status_fields(conn: &Connection, id: &str) -> Option { + conn.query_row( + "SELECT pr_state, pr_checks_status, pr_review_decision, pr_mergeable, + pr_draft, pr_url, pr_updated_at, pr_head_sha + FROM branches WHERE id = ?1", + params![id], + |row| { + Ok(PrStatusFields { + state: row.get(0)?, + checks_status: row.get(1)?, + review_decision: row.get(2)?, + mergeable: row.get::<_, Option>(3)?.map(|b| b != 0), + draft: row.get::<_, Option>(4)?.map(|b| b != 0), + url: row.get(5)?, + updated_at: row.get(6)?, + head_sha: row.get(7)?, + }) + }, + ) + .ok() + } + /// Atomically mark a branch as having completed its initial setup (worktree /// created and prerun actions have had the opportunity to run). /// @@ -213,12 +334,29 @@ impl Store { "UPDATE branches SET setup_complete = 1, updated_at = ?1 WHERE id = ?2 AND setup_complete = 0", params![now_timestamp(), id], )?; + if rows > 0 { + self.publish_with(|| StoreChange::Branch { + branch_id: id.to_string(), + project_id: Self::branch_project_id(&conn, id), + }); + } Ok(rows > 0) } pub fn delete_branch(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); - conn.execute("DELETE FROM branches WHERE id = ?1", params![id])?; + // Resolved before the row disappears, published only if the delete lands. + // A no-op delete (two windows racing to remove the same branch) would + // otherwise publish `project_id: None`, which the frontend reads as its + // widest tier: drop every cached branch list and refetch in every window. + let project_id = Self::branch_project_id(&conn, id); + let rows = conn.execute("DELETE FROM branches WHERE id = ?1", params![id])?; + if rows > 0 { + self.publish(StoreChange::Branch { + branch_id: id.to_string(), + project_id, + }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store/commits.rs b/apps/staged/src-tauri/src/store/commits.rs index 117376c06..1ee14528f 100644 --- a/apps/staged/src-tauri/src/store/commits.rs +++ b/apps/staged/src-tauri/src/store/commits.rs @@ -3,7 +3,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::Commit; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { pub fn create_commit(&self, commit: &Commit) -> Result<(), StoreError> { @@ -20,6 +20,10 @@ impl Store { commit.updated_at, ], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: commit.branch_id.clone(), + project_id: Self::branch_project_id(&conn, &commit.branch_id), + }); Ok(()) } @@ -94,6 +98,14 @@ impl Store { "UPDATE commits SET sha = ?1, updated_at = ?2 WHERE id = ?3", params![sha, now_timestamp(), id], )?; + if let Some(branch_id) = + Self::lookup_id(&conn, "SELECT branch_id FROM commits WHERE id = ?1", id) + { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } Ok(()) } @@ -127,6 +139,10 @@ impl Store { "DELETE FROM commits WHERE id = ?1 AND branch_id = ?2 AND sha IS NULL", params![id, branch_id], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: branch_id.to_string(), + project_id: Self::branch_project_id(&conn, branch_id), + }); return Ok(false); } @@ -134,6 +150,12 @@ impl Store { "UPDATE commits SET sha = ?1, updated_at = ?2 WHERE id = ?3 AND branch_id = ?4 AND sha IS NULL", params![sha, now_timestamp(), id, branch_id], )?; + if rows > 0 { + self.publish_with(|| StoreChange::Branch { + branch_id: branch_id.to_string(), + project_id: Self::branch_project_id(&conn, branch_id), + }); + } Ok(rows > 0) } @@ -186,16 +208,36 @@ impl Store { } tx.commit()?; + if remapped > 0 { + self.publish_with(|| StoreChange::Branch { + branch_id: branch_id.to_string(), + project_id: Self::branch_project_id(&conn, branch_id), + }); + } Ok(remapped) } /// Delete a linked pending commit row if it has not landed. pub fn delete_pending_commit_for_session(&self, session_id: &str) -> Result { let conn = self.conn.lock().unwrap(); + // Resolved before the row disappears, published only if the delete lands. + let branch_id = Self::lookup_id( + &conn, + "SELECT branch_id FROM commits WHERE session_id = ?1 AND sha IS NULL", + session_id, + ); let rows = conn.execute( "DELETE FROM commits WHERE session_id = ?1 AND sha IS NULL", params![session_id], )?; + if rows > 0 { + if let Some(branch_id) = branch_id { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } + } Ok(rows > 0) } @@ -214,7 +256,14 @@ impl Store { pub fn delete_commit(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); + let branch_id = Self::lookup_id(&conn, "SELECT branch_id FROM commits WHERE id = ?1", id); conn.execute("DELETE FROM commits WHERE id = ?1", params![id])?; + if let Some(branch_id) = branch_id { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index bb279b66c..23c570ddd 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use rusqlite::{params, OptionalExtension}; use super::models::Image; -use super::{Store, StoreError}; +use super::{Store, StoreChange, StoreError}; impl Store { pub fn create_image(&self, image: &Image) -> Result<(), StoreError> { @@ -24,6 +24,17 @@ impl Store { image.created_at, ], )?; + // Only branch-attached images without a session appear on the + // timeline; session-scoped attachments live in the chat history, + // which polls. + if image.session_id.is_none() { + if let Some(branch_id) = &image.branch_id { + self.publish_with(|| StoreChange::Branch { + branch_id: branch_id.clone(), + project_id: Some(image.project_id.clone()), + }); + } + } Ok(()) } @@ -149,7 +160,19 @@ impl Store { pub fn delete_image(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); + // Timeline-visible only when branch-attached with no session scope. + let branch_id = Self::lookup_id( + &conn, + "SELECT branch_id FROM images WHERE id = ?1 AND session_id IS NULL", + id, + ); conn.execute("DELETE FROM images WHERE id = ?1", params![id])?; + if let Some(branch_id) = branch_id { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store/mod.rs b/apps/staged/src-tauri/src/store/mod.rs index c8e519a11..ab8fde9f5 100644 --- a/apps/staged/src-tauri/src/store/mod.rs +++ b/apps/staged/src-tauri/src/store/mod.rs @@ -46,6 +46,51 @@ pub use repo_badges::{fallback_short_name, next_hue}; use rusqlite::{Connection, OptionalExtension}; use std::path::{Path, PathBuf}; use std::sync::Mutex; +use tokio::sync::broadcast; + +// ============================================================================= +// StoreChange +// ============================================================================= + +/// A domain-level change published by every mutating `Store` method. +/// +/// The variants speak the frontend's vocabulary (projects, branches, notes, +/// reviews, repos) — never table names. Each carries the aggregate id the +/// mutation already had in scope; secondary ids are filled best-effort and +/// may be `None`, in which case consumers fall back to a broader refetch. +/// +/// Sessions are deliberately absent: chat polls at 500ms and session +/// lifecycle already flows through `session-status-changed` & friends, so +/// session-family writes (sessions, messages, queued messages, session-scoped +/// images, repo actions) publish nothing here. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum StoreChange { + /// A project row or its attached `project_repos` changed. + Project { project_id: Option }, + /// A branch or anything on its timeline (commits, workdir assignment, + /// branch-attached images) changed. + /// + /// `project_id` means "this project's branch list is affected", not "this + /// branch's current parent", so a mutation touching more than one project's + /// list publishes once per project: a move names both the target and the + /// source. + Branch { + branch_id: String, + project_id: Option, + }, + /// A branch note or project note changed. + Notes { + branch_id: Option, + project_id: Option, + }, + /// A review or its comments / reviewed files / reference files changed. + Review { + review_id: String, + branch_id: Option, + }, + /// Repo badges, recent repos, or repo affinities changed. + Repos { github_repo: Option }, +} // ============================================================================= // Error type @@ -207,6 +252,11 @@ pub struct ResolvedSession { pub struct Store { conn: Mutex, + /// Change feed for mutating methods. `None` (the default, and what unit + /// tests get) makes every publish a no-op; the app wires a sender in at + /// construction so a task above the Tauri boundary can forward changes + /// to all windows and web clients. + change_tx: Option>, } impl Store { @@ -219,22 +269,72 @@ impl Store { let conn = Connection::open(path)?; let store = Self { conn: Mutex::new(conn), + change_tx: None, }; store.init_schema()?; Ok(store) } + /// Attach the change feed sender. Keeps `Store` Tauri-agnostic: it only + /// knows it publishes [`StoreChange`]s, not who listens. + pub fn with_change_sender(mut self, tx: broadcast::Sender) -> Self { + self.change_tx = Some(tx); + self + } + /// In-memory database for testing. #[cfg(test)] pub fn in_memory() -> Result { let conn = Connection::open_in_memory()?; let store = Self { conn: Mutex::new(conn), + change_tx: None, }; store.init_schema()?; Ok(store) } + /// Publish a change to the feed, if one is attached. + /// + /// Never blocks and never fails the mutation: a send error just means + /// nobody is listening right now. + pub(crate) fn publish(&self, change: StoreChange) { + if let Some(tx) = &self.change_tx { + let _ = tx.send(change); + } + } + + /// Like [`Store::publish`] but lazy — `make` (and any payload-enrichment + /// query inside it) only runs when a change feed is attached. + pub(crate) fn publish_with(&self, make: impl FnOnce() -> StoreChange) { + if let Some(tx) = &self.change_tx { + let _ = tx.send(make()); + } + } + + /// Best-effort single-id lookup for change payload enrichment. + /// + /// `sql` must select exactly one (possibly NULL) TEXT column keyed by + /// `?1`. Returns `None` on any miss or error — a missing secondary id + /// degrades to a broader frontend refetch, never a failed write. + pub(crate) fn lookup_id(conn: &Connection, sql: &str, key: &str) -> Option { + conn.query_row(sql, [key], |row| row.get::<_, Option>(0)) + .optional() + .ok() + .flatten() + .flatten() + } + + /// A branch's `project_id`, for [`StoreChange::Branch`] payload + /// enrichment from modules that only hold the branch id. + pub(crate) fn branch_project_id(conn: &Connection, branch_id: &str) -> Option { + Self::lookup_id( + conn, + "SELECT project_id FROM branches WHERE id = ?1", + branch_id, + ) + } + /// Resolve an optional session_id into its associated metadata. /// /// Returns a default (all-`None`) [`ResolvedSession`] when `session_id` is diff --git a/apps/staged/src-tauri/src/store/notes.rs b/apps/staged/src-tauri/src/store/notes.rs index 5736e4983..61421029b 100644 --- a/apps/staged/src-tauri/src/store/notes.rs +++ b/apps/staged/src-tauri/src/store/notes.rs @@ -3,12 +3,17 @@ use rusqlite::{params, OptionalExtension}; use super::models::Note; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { pub fn create_note(&self, note: &Note) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); - Self::insert_note(&conn, note) + Self::insert_note(&conn, note)?; + self.publish(StoreChange::Notes { + branch_id: Some(note.branch_id.clone()), + project_id: None, + }); + Ok(()) } /// Like [`Store::create_note`], but if the requested title collides with @@ -24,7 +29,12 @@ impl Store { if !note.title.is_empty() { note.title = Self::resolve_unique_note_title(&conn, ¬e.branch_id, ¬e.title)?; } - Self::insert_note(&conn, note) + Self::insert_note(&conn, note)?; + self.publish(StoreChange::Notes { + branch_id: Some(note.branch_id.clone()), + project_id: None, + }); + Ok(()) } fn insert_note(conn: &rusqlite::Connection, note: &Note) -> Result<(), StoreError> { @@ -195,6 +205,10 @@ impl Store { "UPDATE notes SET title = ?1, content = ?2, updated_at = ?3, completed_at = COALESCE(completed_at, ?4), suggested_next_commit_step = ?5, suggested_next_note_step = ?6 WHERE id = ?7", params![title, content, now, now, suggested_next_commit_step, suggested_next_note_step, id], )?; + self.publish_with(|| StoreChange::Notes { + branch_id: Self::lookup_id(&conn, "SELECT branch_id FROM notes WHERE id = ?1", id), + project_id: None, + }); Ok(()) } @@ -205,6 +219,10 @@ impl Store { "UPDATE notes SET content = ?1, updated_at = ?2, completed_at = COALESCE(completed_at, ?3) WHERE id = ?4", params![content, now, now, id], )?; + self.publish_with(|| StoreChange::Notes { + branch_id: Self::lookup_id(&conn, "SELECT branch_id FROM notes WHERE id = ?1", id), + project_id: None, + }); Ok(()) } @@ -215,12 +233,24 @@ impl Store { "UPDATE notes SET completed_at = COALESCE(completed_at, ?1) WHERE id = ?2", params![now, id], )?; + self.publish_with(|| StoreChange::Notes { + branch_id: Self::lookup_id(&conn, "SELECT branch_id FROM notes WHERE id = ?1", id), + project_id: None, + }); Ok(()) } pub fn delete_note(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); + // Resolved before the row disappears, published only if the delete lands. + let branch_id = Self::lookup_id(&conn, "SELECT branch_id FROM notes WHERE id = ?1", id); conn.execute("DELETE FROM notes WHERE id = ?1", params![id])?; + if branch_id.is_some() { + self.publish(StoreChange::Notes { + branch_id, + project_id: None, + }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store/project_notes.rs b/apps/staged/src-tauri/src/store/project_notes.rs index a890b5398..e4b461504 100644 --- a/apps/staged/src-tauri/src/store/project_notes.rs +++ b/apps/staged/src-tauri/src/store/project_notes.rs @@ -3,7 +3,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::ProjectNote; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; /// Sessions left behind by [`Store::delete_project_note`]. /// @@ -48,6 +48,10 @@ impl Store { note.suggested_next_note_step, ], )?; + self.publish(StoreChange::Notes { + branch_id: None, + project_id: Some(note.project_id.clone()), + }); Ok(()) } @@ -144,6 +148,14 @@ impl Store { WHERE id = ?7", params![title, content, now, now, suggested_next_commit_step, suggested_next_note_step, id], )?; + self.publish_with(|| StoreChange::Notes { + branch_id: None, + project_id: Self::lookup_id( + &conn, + "SELECT project_id FROM project_notes WHERE id = ?1", + id, + ), + }); Ok(()) } @@ -154,6 +166,14 @@ impl Store { "UPDATE project_notes SET completed_at = COALESCE(completed_at, ?1) WHERE id = ?2", params![now, id], )?; + self.publish_with(|| StoreChange::Notes { + branch_id: None, + project_id: Self::lookup_id( + &conn, + "SELECT project_id FROM project_notes WHERE id = ?1", + id, + ), + }); Ok(()) } @@ -182,16 +202,27 @@ impl Store { "DELETE FROM notes WHERE parent_project_note_id = ?1", params![id], )?; - let session_id: Option> = tx + let deleted: Option<(Option, String)> = tx .query_row( - "DELETE FROM project_notes WHERE id = ?1 RETURNING session_id", + "DELETE FROM project_notes WHERE id = ?1 RETURNING session_id, project_id", params![id], - |row| row.get(0), + |row| Ok((row.get(0)?, row.get(1)?)), ) .optional()?; tx.commit()?; + + let (project_note_session_id, project_id) = match deleted { + Some((session_id, project_id)) => (session_id, Some(project_id)), + None => (None, None), + }; + if project_id.is_some() || !child_session_ids.is_empty() { + self.publish(StoreChange::Notes { + branch_id: None, + project_id, + }); + } Ok(DeletedProjectNoteSessions { - project_note_session_id: session_id.flatten(), + project_note_session_id, child_session_ids, }) } diff --git a/apps/staged/src-tauri/src/store/project_repos.rs b/apps/staged/src-tauri/src/store/project_repos.rs index 1387f39eb..803d1e580 100644 --- a/apps/staged/src-tauri/src/store/project_repos.rs +++ b/apps/staged/src-tauri/src/store/project_repos.rs @@ -3,7 +3,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::ProjectRepo; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { pub fn create_project_repo(&self, repo: &ProjectRepo) -> Result<(), StoreError> { @@ -39,6 +39,9 @@ impl Store { } return Err(e.into()); } + self.publish(StoreChange::Project { + project_id: Some(repo.project_id.clone()), + }); Ok(()) } @@ -92,6 +95,9 @@ impl Store { "UPDATE project_repos SET branch_name = ?1, updated_at = ?2 WHERE id = ?3 AND project_id = ?4", params![branch_name, now_timestamp(), repo_id, project_id], )?; + self.publish(StoreChange::Project { + project_id: Some(project_id.to_string()), + }); Ok(()) } @@ -110,6 +116,9 @@ impl Store { "UPDATE project_repos SET is_primary = 1, updated_at = ?1 WHERE id = ?2 AND project_id = ?3", params![now, repo_id, project_id], )?; + self.publish(StoreChange::Project { + project_id: Some(project_id.to_string()), + }); Ok(()) } @@ -119,12 +128,31 @@ impl Store { "UPDATE project_repos SET reason = NULL, updated_at = ?1 WHERE id = ?2", params![now_timestamp(), repo_id], )?; + self.publish_with(|| StoreChange::Project { + project_id: Self::lookup_id( + &conn, + "SELECT project_id FROM project_repos WHERE id = ?1", + repo_id, + ), + }); Ok(()) } pub fn delete_project_repo(&self, repo_id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); - conn.execute("DELETE FROM project_repos WHERE id = ?1", params![repo_id])?; + // Resolved before the row disappears, published only if the delete lands. + // A no-op delete would otherwise publish `project_id: None`, which the + // frontend reads as its widest tier: drop every cached project-repo list + // and refetch in every window. + let project_id = Self::lookup_id( + &conn, + "SELECT project_id FROM project_repos WHERE id = ?1", + repo_id, + ); + let rows = conn.execute("DELETE FROM project_repos WHERE id = ?1", params![repo_id])?; + if rows > 0 { + self.publish(StoreChange::Project { project_id }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store/projects.rs b/apps/staged/src-tauri/src/store/projects.rs index 9d770d369..ae6386fe6 100644 --- a/apps/staged/src-tauri/src/store/projects.rs +++ b/apps/staged/src-tauri/src/store/projects.rs @@ -3,7 +3,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::Project; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { pub fn create_project(&self, project: &Project) -> Result<(), StoreError> { @@ -21,6 +21,9 @@ impl Store { project.updated_at, ], )?; + self.publish(StoreChange::Project { + project_id: Some(project.id.clone()), + }); Ok(()) } @@ -129,12 +132,18 @@ impl Store { "UPDATE projects SET name = ?1, github_repo = ?2, location = ?3, subpath = ?4, updated_at = ?5 WHERE id = ?6", params![name, github_repo, location.as_str(), subpath, now_timestamp(), id], )?; + self.publish(StoreChange::Project { + project_id: Some(id.to_string()), + }); Ok(()) } pub fn delete_project(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); conn.execute("DELETE FROM projects WHERE id = ?1", params![id])?; + self.publish(StoreChange::Project { + project_id: Some(id.to_string()), + }); Ok(()) } } diff --git a/apps/staged/src-tauri/src/store/recent_repos.rs b/apps/staged/src-tauri/src/store/recent_repos.rs index fffc02837..98fed0076 100644 --- a/apps/staged/src-tauri/src/store/recent_repos.rs +++ b/apps/staged/src-tauri/src/store/recent_repos.rs @@ -3,7 +3,7 @@ use rusqlite::params; use super::models::RecentRepo; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { /// Record that a repository was used, keeping only the most recent 20. @@ -35,6 +35,9 @@ impl Store { [], )?; + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } diff --git a/apps/staged/src-tauri/src/store/repo_affinities.rs b/apps/staged/src-tauri/src/store/repo_affinities.rs index 5b8898612..f3d2d05cf 100644 --- a/apps/staged/src-tauri/src/store/repo_affinities.rs +++ b/apps/staged/src-tauri/src/store/repo_affinities.rs @@ -3,7 +3,7 @@ use rusqlite::params; use super::models::SuggestedRepo; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; /// Build the canonical affinity key for a repo, encoding the subpath when present. /// The `::` delimiter is safe because neither GitHub `owner/repo` names nor @@ -47,6 +47,8 @@ impl Store { last_seen_at = ?3", params![a, b, now_timestamp()], )?; + // Affinity keys are composite (`repo::subpath`); no single repo to name. + self.publish(StoreChange::Repos { github_repo: None }); Ok(()) } diff --git a/apps/staged/src-tauri/src/store/repo_badges.rs b/apps/staged/src-tauri/src/store/repo_badges.rs index f6f16b9f9..b19d25bcc 100644 --- a/apps/staged/src-tauri/src/store/repo_badges.rs +++ b/apps/staged/src-tauri/src/store/repo_badges.rs @@ -3,7 +3,7 @@ use rusqlite::params; use super::models::RepoBadge; -use super::{Store, StoreError}; +use super::{Store, StoreChange, StoreError}; /// All columns selected in badge queries, in a fixed order. const BADGE_COLUMNS: &str = @@ -75,6 +75,9 @@ impl Store { badge.default_branch, ], )?; + self.publish(StoreChange::Repos { + github_repo: Some(badge.github_repo.clone()), + }); Ok(()) } @@ -98,6 +101,9 @@ impl Store { "No badge found for {github_repo} subpath={subpath}" ))); } + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } @@ -124,6 +130,9 @@ impl Store { "DELETE FROM repo_badges WHERE github_repo = ?1 AND subpath = ?2", params![github_repo, subpath], )?; + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } @@ -158,6 +167,9 @@ impl Store { "No badge found for {github_repo} subpath={subpath}" ))); } + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } @@ -174,6 +186,9 @@ impl Store { "No badge found for {github_repo} subpath={subpath}" ))); } + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } @@ -192,6 +207,8 @@ impl Store { )?; } tx.commit()?; + // Bulk reorder — no single repo to name. + self.publish(StoreChange::Repos { github_repo: None }); Ok(()) } @@ -258,6 +275,9 @@ impl Store { "No badge found for {github_repo} subpath={subpath}" ))); } + self.publish(StoreChange::Repos { + github_repo: Some(github_repo.to_string()), + }); Ok(()) } } diff --git a/apps/staged/src-tauri/src/store/reviews.rs b/apps/staged/src-tauri/src/store/reviews.rs index 2b6cb0622..1dc6b17f9 100644 --- a/apps/staged/src-tauri/src/store/reviews.rs +++ b/apps/staged/src-tauri/src/store/reviews.rs @@ -5,7 +5,7 @@ use rusqlite::{params, Connection, OptionalExtension}; use crate::git::Span; use super::models::{Comment, CommentType, Review, ReviewScope}; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { /// Create a new review. @@ -26,6 +26,10 @@ impl Store { review.completed_at, ], )?; + self.publish(StoreChange::Review { + review_id: review.id.clone(), + branch_id: Some(review.branch_id.clone()), + }); Ok(()) } @@ -80,6 +84,10 @@ impl Store { review.completed_at, ], )?; + self.publish(StoreChange::Review { + review_id: review.id.clone(), + branch_id: Some(review.branch_id.clone()), + }); Ok(review) } @@ -160,6 +168,7 @@ impl Store { params![review_id, path], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -171,6 +180,7 @@ impl Store { params![review_id, path], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -193,6 +203,7 @@ impl Store { ], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -204,7 +215,9 @@ impl Store { "UPDATE comments SET content = ?1, github_comment_stale = CASE WHEN github_comment_id IS NOT NULL THEN 1 ELSE 0 END WHERE id = ?2", params![content, comment_id], )?; - Self::touch_review_for_comment(&conn, comment_id)?; + if let Some(review_id) = Self::touch_review_for_comment(&conn, comment_id)? { + self.publish_review_changed(&conn, &review_id); + } Ok(()) } @@ -220,6 +233,9 @@ impl Store { "UPDATE comments SET github_comment_id = ?1, github_comment_type = ?2, github_comment_stale = 0 WHERE id = ?3", params![github_id, github_type, comment_id], )?; + if let Some(review_id) = Self::comment_review_id(&conn, comment_id) { + self.publish_review_changed(&conn, &review_id); + } Ok(()) } @@ -246,6 +262,9 @@ impl Store { )?, other => return Err(StoreError(format!("Invalid comment session type: {other}"))), }; + if let Some(review_id) = Self::comment_review_id(&conn, comment_id) { + self.publish_review_changed(&conn, &review_id); + } Ok(()) } @@ -257,7 +276,9 @@ impl Store { "UPDATE comments SET deleted_at = ?1 WHERE id = ?2 AND deleted_at IS NULL", params![now, comment_id], )?; - Self::touch_review_for_comment(&conn, comment_id)?; + if let Some(review_id) = Self::touch_review_for_comment(&conn, comment_id)? { + self.publish_review_changed(&conn, &review_id); + } Ok(()) } @@ -270,6 +291,7 @@ impl Store { params![now, review_id], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -280,7 +302,9 @@ impl Store { "UPDATE comments SET deleted_at = NULL WHERE id = ?1 AND deleted_at IS NOT NULL", params![comment_id], )?; - Self::touch_review_for_comment(&conn, comment_id)?; + if let Some(review_id) = Self::touch_review_for_comment(&conn, comment_id)? { + self.publish_review_changed(&conn, &review_id); + } Ok(()) } @@ -324,6 +348,7 @@ impl Store { params![review_id, path], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -335,6 +360,7 @@ impl Store { params![review_id, path], )?; Self::touch_review(&conn, review_id)?; + self.publish_review_changed(&conn, review_id); Ok(()) } @@ -362,7 +388,19 @@ impl Store { /// Delete an entire review and all associated data (cascades). pub fn delete_review(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); - conn.execute("DELETE FROM reviews WHERE id = ?1", params![id])?; + let branch_id: Option = conn + .query_row( + "DELETE FROM reviews WHERE id = ?1 RETURNING branch_id", + params![id], + |row| row.get(0), + ) + .optional()?; + if branch_id.is_some() { + self.publish(StoreChange::Review { + review_id: id.to_string(), + branch_id, + }); + } Ok(()) } @@ -394,6 +432,7 @@ impl Store { "UPDATE reviews SET completed_at = COALESCE(completed_at, ?1) WHERE id = ?2", params![now, id], )?; + self.publish_review_changed(&conn, id); Ok(()) } @@ -405,6 +444,7 @@ impl Store { "UPDATE reviews SET title = ?1, updated_at = ?2, completed_at = COALESCE(completed_at, ?3) WHERE id = ?4", params![title, now, now, id], )?; + self.publish_review_changed(&conn, id); Ok(()) } @@ -416,6 +456,7 @@ impl Store { "UPDATE reviews SET commit_sha = ?1, updated_at = ?2 WHERE id = ?3", params![commit_sha, now, id], )?; + self.publish_review_changed(&conn, id); Ok(()) } @@ -432,18 +473,37 @@ impl Store { } /// Look up the parent review for a comment and touch its `updated_at`. - fn touch_review_for_comment(conn: &Connection, comment_id: &str) -> Result<(), StoreError> { - let review_id: Option = conn - .query_row( - "SELECT review_id FROM comments WHERE id = ?1", - params![comment_id], - |row| row.get(0), - ) - .optional()?; - if let Some(rid) = review_id { - Self::touch_review(conn, &rid)?; + /// Returns the review id so callers can publish a change for it. + fn touch_review_for_comment( + conn: &Connection, + comment_id: &str, + ) -> Result, StoreError> { + let review_id = Self::comment_review_id(conn, comment_id); + if let Some(rid) = &review_id { + Self::touch_review(conn, rid)?; } - Ok(()) + Ok(review_id) + } + + /// The parent review of a comment, best-effort. + fn comment_review_id(conn: &Connection, comment_id: &str) -> Option { + Self::lookup_id( + conn, + "SELECT review_id FROM comments WHERE id = ?1", + comment_id, + ) + } + + /// Publish a [`StoreChange::Review`] with best-effort branch enrichment. + fn publish_review_changed(&self, conn: &Connection, review_id: &str) { + self.publish_with(|| StoreChange::Review { + review_id: review_id.to_string(), + branch_id: Self::lookup_id( + conn, + "SELECT branch_id FROM reviews WHERE id = ?1", + review_id, + ), + }); } fn row_to_review_header(row: &rusqlite::Row) -> rusqlite::Result { diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index e87fc4b51..c3cb59b77 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -2995,3 +2995,308 @@ fn test_delete_branch_cascades_reviews() { store.delete_branch(&branch.id).unwrap(); assert!(store.get_review(&review.id).unwrap().is_none()); } + +// ============================================================================= +// Change feed +// ============================================================================= + +#[test] +fn change_feed_publishes_domain_changes_for_mutations() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Project { + project_id: Some(project.id.clone()) + } + ); + + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()) + } + ); + + // Methods that only hold the aggregate id enrich the secondary id by lookup. + store.update_branch_name(&branch.id, "feature-2").unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()) + } + ); + + // Deletes resolve enrichment before the row disappears. + store.delete_branch(&branch.id).unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()) + } + ); +} + +#[test] +fn change_feed_ensure_review_publishes_only_on_create() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + while rx.try_recv().is_ok() {} + + let review = store + .ensure_review(&branch.id, "abc123", ReviewScope::Commit) + .unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Review { + review_id: review.id.clone(), + branch_id: Some(branch.id.clone()) + } + ); + + // Second ensure finds the existing review — no write, no event. + let again = store + .ensure_review(&branch.id, "abc123", ReviewScope::Commit) + .unwrap(); + assert_eq!(again.id, review.id); + assert!(rx.try_recv().is_err()); +} + +#[test] +fn change_feed_pr_status_publishes_only_on_domain_change() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + while rx.try_recv().is_ok() {} + + let expected = super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()), + }; + let poll = |checks: &str| { + store + .update_branch_pr_status( + &branch.id, + Some("OPEN".to_string()), + Some(checks.to_string()), + Some("APPROVED".to_string()), + Some(true), + Some(false), + Some("https://github.com/test-owner/test-repo/pull/1".to_string()), + Some(1_700_000_000), + Some("abc123".to_string()), + ) + .unwrap(); + }; + + // First fetch moves the domain fields off their defaults. + poll("PENDING"); + assert_eq!(rx.try_recv().unwrap(), expected); + + // The steady state: the poller refetches identical PR state. Only + // pr_fetched_at / updated_at move, so the feed stays silent. + poll("PENDING"); + assert!(rx.try_recv().is_err()); + + // A genuine flip publishes exactly one event. + poll("SUCCESS"); + assert_eq!(rx.try_recv().unwrap(), expected); + assert!(rx.try_recv().is_err()); + + // The clear path (PR gone) nulls the fields — also a real change. + store + .update_branch_pr_status(&branch.id, None, None, None, None, None, None, None, None) + .unwrap(); + assert_eq!(rx.try_recv().unwrap(), expected); + + // Clearing an already-cleared branch is a no-op poll echo. + store + .update_branch_pr_status(&branch.id, None, None, None, None, None, None, None, None) + .unwrap(); + assert!(rx.try_recv().is_err()); + + // A branch that no longer exists publishes nothing. + store.delete_branch(&branch.id).unwrap(); + while rx.try_recv().is_ok() {} + poll("SUCCESS"); + assert!(rx.try_recv().is_err()); +} + +#[test] +fn change_feed_workspace_status_publishes_only_on_domain_change() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + // A local branch starts with a NULL workspace_status, so the first real + // status exercises the null-safe half of the `IS NOT` guard. + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + while rx.try_recv().is_ok() {} + + let expected = super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()), + }; + // Returns the stored status, so every assertion below pins the write as + // well as the publish. + let poll = |status: WorkspaceStatus| { + store + .update_branch_workspace_status(&branch.id, &status) + .unwrap(); + store + .get_branch(&branch.id) + .unwrap() + .unwrap() + .workspace_status + }; + + // NULL -> Starting is a real change. + assert_eq!( + poll(WorkspaceStatus::Starting), + Some(WorkspaceStatus::Starting) + ); + assert_eq!(rx.try_recv().unwrap(), expected); + + // The steady state: the Blox poller rewrites the status it already sees. + // The status is still stored, so the silence is "nothing moved", not "a + // real change went missing". + assert_eq!( + poll(WorkspaceStatus::Starting), + Some(WorkspaceStatus::Starting) + ); + assert!(rx.try_recv().is_err()); + + // A genuine transition publishes exactly one event. + assert_eq!( + poll(WorkspaceStatus::Running), + Some(WorkspaceStatus::Running) + ); + assert_eq!(rx.try_recv().unwrap(), expected); + assert!(rx.try_recv().is_err()); + + // ...and its own poll echo is silent again. + assert_eq!( + poll(WorkspaceStatus::Running), + Some(WorkspaceStatus::Running) + ); + assert!(rx.try_recv().is_err()); + + // A branch that no longer exists matches no row, so it publishes nothing. + store.delete_branch(&branch.id).unwrap(); + while rx.try_recv().is_ok() {} + store + .update_branch_workspace_status(&branch.id, &WorkspaceStatus::Stopped) + .unwrap(); + assert!(rx.try_recv().is_err()); +} + +#[test] +fn change_feed_workspace_status_by_name_publishes_only_for_moved_branches() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let already = Branch::new_remote(&project.id, "peer-a", "main", "shared-ws"); + let moving = Branch::new_remote(&project.id, "peer-b", "main", "shared-ws"); + store.create_branch(&already).unwrap(); + store.create_branch(&moving).unwrap(); + // Both are created Starting, so park one elsewhere to give it somewhere to + // move back from. + store + .update_branch_workspace_status(&moving.id, &WorkspaceStatus::Suspended) + .unwrap(); + while rx.try_recv().is_ok() {} + + let ids = store + .update_workspace_status_by_workspace_name("shared-ws", &WorkspaceStatus::Starting) + .unwrap(); + + // The return value still covers every branch on the workspace, moved or + // not — `resume_workspace` reads an empty vec as "no such workspace" and + // turns it into a user-visible error. + let mut returned = ids.clone(); + returned.sort(); + let mut all_ids = vec![already.id.clone(), moving.id.clone()]; + all_ids.sort(); + assert_eq!(returned, all_ids); + + // Only the branch whose status actually moved publishes. + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Branch { + branch_id: moving.id.clone(), + project_id: Some(project.id.clone()), + } + ); + assert!(rx.try_recv().is_err()); + + // Both rows hold the new status, so the silence was a no-op and not a + // skipped write. + for id in &ids { + assert_eq!( + store.get_branch(id).unwrap().unwrap().workspace_status, + Some(WorkspaceStatus::Starting) + ); + } +} + +#[test] +fn change_feed_deletes_publish_only_when_a_row_is_removed() { + let (tx, mut rx) = tokio::sync::broadcast::channel(64); + let store = Store::in_memory().unwrap().with_change_sender(tx); + + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + let repo = ProjectRepo::new(&project.id, "test-owner/test-repo", "main", None).primary(); + store.create_project_repo(&repo).unwrap(); + while rx.try_recv().is_ok() {} + + // The delete that lands carries the enrichment resolved before the row went. + store.delete_branch(&branch.id).unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Branch { + branch_id: branch.id.clone(), + project_id: Some(project.id.clone()), + } + ); + + // Two windows racing to delete the same branch: the loser matches no row. + // Publishing here would carry `project_id: None`, the frontend's widest + // tier — every cached branch list dropped and refetched in every window. + store.delete_branch(&branch.id).unwrap(); + assert!(rx.try_recv().is_err()); + + store.delete_project_repo(&repo.id).unwrap(); + assert_eq!( + rx.try_recv().unwrap(), + super::StoreChange::Project { + project_id: Some(project.id.clone()), + } + ); + + store.delete_project_repo(&repo.id).unwrap(); + assert!(rx.try_recv().is_err()); +} diff --git a/apps/staged/src-tauri/src/store/workdirs.rs b/apps/staged/src-tauri/src/store/workdirs.rs index 560485ecf..6213afc4e 100644 --- a/apps/staged/src-tauri/src/store/workdirs.rs +++ b/apps/staged/src-tauri/src/store/workdirs.rs @@ -3,7 +3,7 @@ use rusqlite::{params, OptionalExtension}; use super::models::Workdir; -use super::{now_timestamp, Store, StoreError}; +use super::{now_timestamp, Store, StoreChange, StoreError}; impl Store { pub fn create_workdir(&self, workdir: &Workdir) -> Result<(), StoreError> { @@ -20,6 +20,13 @@ impl Store { workdir.updated_at, ], )?; + // A workdir is only UI-visible through the branch occupying it. + if let Some(branch_id) = &workdir.branch_id { + self.publish(StoreChange::Branch { + branch_id: branch_id.clone(), + project_id: Some(workdir.project_id.clone()), + }); + } Ok(()) } @@ -80,22 +87,46 @@ impl Store { "UPDATE workdirs SET branch_id = ?1, updated_at = ?2 WHERE id = ?3", params![branch_id, now_timestamp(), workdir_id], )?; + self.publish_with(|| StoreChange::Branch { + branch_id: branch_id.to_string(), + project_id: Self::branch_project_id(&conn, branch_id), + }); Ok(()) } /// Release a workdir (clear its branch assignment). pub fn release_workdir(&self, workdir_id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); + // The branch losing its workdir is the visible change; resolve it + // before the assignment is cleared. + let branch_id = Self::lookup_id( + &conn, + "SELECT branch_id FROM workdirs WHERE id = ?1", + workdir_id, + ); conn.execute( "UPDATE workdirs SET branch_id = NULL, updated_at = ?1 WHERE id = ?2", params![now_timestamp(), workdir_id], )?; + if let Some(branch_id) = branch_id { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } Ok(()) } pub fn delete_workdir(&self, id: &str) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); + let branch_id = Self::lookup_id(&conn, "SELECT branch_id FROM workdirs WHERE id = ?1", id); conn.execute("DELETE FROM workdirs WHERE id = ?1", params![id])?; + if let Some(branch_id) = branch_id { + self.publish_with(|| StoreChange::Branch { + project_id: Self::branch_project_id(&conn, &branch_id), + branch_id, + }); + } Ok(()) } diff --git a/apps/staged/src-tauri/src/store_events.rs b/apps/staged/src-tauri/src/store_events.rs new file mode 100644 index 000000000..c3277654d --- /dev/null +++ b/apps/staged/src-tauri/src/store_events.rs @@ -0,0 +1,331 @@ +//! Store change feed → frontend domain events. +//! +//! [`Store`](crate::store::Store) publishes a [`StoreChange`] from every +//! mutating method. This module is the piece above the Tauri boundary: a +//! task that drains the broadcast channel, coalesces bursts, and forwards +//! each distinct change through [`emit_to_all`] so every window and every +//! web client sees the same event on the same wire. +//! +//! Event names and payloads (all ids camelCase, `null` when unknown — +//! consumers treat a missing id as "refetch the whole surface"): +//! +//! | `StoreChange` | event | payload | +//! |---------------|------------------|------------------------------| +//! | `Project` | `project-changed`| `{ projectId }` | +//! | `Branch` | `branch-changed` | `{ branchId, projectId }` | +//! | `Notes` | `notes-changed` | `{ branchId, projectId }` | +//! | `Review` | `review-changed` | `{ reviewId, branchId }` | +//! | `Repos` | `repos-changed` | `{ githubRepo }` | +//! +//! If the receiver falls behind the channel's capacity, the missed changes +//! are gone — so instead of dropping them silently, the task emits +//! [`lag_flush_events`]: every event with every id `null`, i.e. "the feed +//! lost track, refetch everything". See [`run`]. + +use crate::store::StoreChange; +use crate::web_server::emit_to_all; +use serde_json::json; +use std::collections::HashSet; +use std::time::Duration; +use tokio::sync::broadcast; + +/// How long to keep absorbing further changes after the first one arrives +/// before flushing the batch. Long enough that a bulk write (deleting a +/// project's branches, a multi-repo setup) collapses its duplicates into +/// one event each; short enough to be imperceptible after a single +/// interactive write. +const COALESCE_WINDOW: Duration = Duration::from_millis(50); + +/// Spawn the forwarding task. Runs for the life of the app; survives a +/// store reset because the channel does. +pub fn spawn(app_handle: tauri::AppHandle, rx: broadcast::Receiver) { + tauri::async_runtime::spawn(async move { + run(rx, move |event, payload| { + emit_to_all(&app_handle, event, payload); + }) + .await; + }); +} + +/// Drain, coalesce, and forward until the channel closes. +/// +/// Split out of [`spawn`] so tests can drive it with a plain closure instead +/// of an `AppHandle`. +async fn run( + mut rx: broadcast::Receiver, + mut emit: impl FnMut(&'static str, serde_json::Value), +) { + loop { + // Set by either receive arm: the batch can no longer describe what + // changed, so the window flushes the all-null recovery events instead. + let mut lagged = false; + + // Idle until something changes, then open the coalescing window. + let first = match rx.recv().await { + Ok(change) => Some(change), + Err(broadcast::error::RecvError::Lagged(missed)) => { + log::warn!( + "Store change feed lagged; {missed} change(s) dropped — \ + flushing a full invalidation" + ); + // Fall through into the window rather than looping: it lets + // the burst that caused the lag finish, so one flush covers it. + lagged = true; + None + } + Err(broadcast::error::RecvError::Closed) => return, + }; + + // First-seen order, deduplicated on the full change (variant + ids). + let mut batch = Vec::new(); + let mut seen = HashSet::new(); + if let Some(change) = first { + seen.insert(change.clone()); + batch.push(change); + } + let mut closed = false; + + let window = tokio::time::sleep(COALESCE_WINDOW); + tokio::pin!(window); + loop { + tokio::select! { + _ = &mut window => break, + recv = rx.recv() => match recv { + Ok(change) => { + // Once lagged, individual changes are subsumed by the + // null flush — keep draining (which keeps the receiver + // caught up) without accumulating them. + if !lagged && seen.insert(change.clone()) { + batch.push(change); + } + } + Err(broadcast::error::RecvError::Lagged(missed)) => { + log::warn!( + "Store change feed lagged; {missed} change(s) dropped — \ + flushing a full invalidation" + ); + lagged = true; + batch.clear(); + seen.clear(); + } + Err(broadcast::error::RecvError::Closed) => { + closed = true; + break; + } + }, + } + } + + // Every change the burst dropped had already committed before it was + // published, so the refetches these events trigger read post-burst + // state. + if lagged { + for (event, payload) in lag_flush_events() { + emit(event, payload); + } + } else { + for change in batch { + let (event, payload) = event_for(&change); + emit(event, payload); + } + } + if closed { + return; + } + } +} + +/// The recovery flush: one event per aggregate carrying only nulls, which +/// consumers read as "refetch the whole surface". +/// +/// Synthesized directly as wire payloads because [`StoreChange`] can't +/// represent them — a real mutation always knows its aggregate id, and +/// loosening the enum would weaken that invariant across every publish site. +/// So an all-null payload means exactly one thing: the feed lost track. +fn lag_flush_events() -> [(&'static str, serde_json::Value); 5] { + [ + ("project-changed", json!({ "projectId": null })), + ( + "branch-changed", + json!({ "branchId": null, "projectId": null }), + ), + ( + "notes-changed", + json!({ "branchId": null, "projectId": null }), + ), + ( + "review-changed", + json!({ "reviewId": null, "branchId": null }), + ), + ("repos-changed", json!({ "githubRepo": null })), + ] +} + +/// Map a change to its wire event name and payload. +fn event_for(change: &StoreChange) -> (&'static str, serde_json::Value) { + match change { + StoreChange::Project { project_id } => { + ("project-changed", json!({ "projectId": project_id })) + } + StoreChange::Branch { + branch_id, + project_id, + } => ( + "branch-changed", + json!({ "branchId": branch_id, "projectId": project_id }), + ), + StoreChange::Notes { + branch_id, + project_id, + } => ( + "notes-changed", + json!({ "branchId": branch_id, "projectId": project_id }), + ), + StoreChange::Review { + review_id, + branch_id, + } => ( + "review-changed", + json!({ "reviewId": review_id, "branchId": branch_id }), + ), + StoreChange::Repos { github_repo } => { + ("repos-changed", json!({ "githubRepo": github_repo })) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::Value; + use std::sync::{Arc, Mutex}; + + type Emitted = Arc>>; + + /// A recording `emit` closure plus the handle to read it back. + fn collector() -> (Emitted, impl FnMut(&'static str, Value)) { + let emitted: Emitted = Arc::new(Mutex::new(Vec::new())); + let sink = emitted.clone(); + (emitted, move |event, payload| { + sink.lock().unwrap().push((event, payload)); + }) + } + + fn branch(id: &str) -> StoreChange { + StoreChange::Branch { + branch_id: id.to_string(), + project_id: Some("p1".to_string()), + } + } + + #[tokio::test(start_paused = true)] + async fn lag_before_the_window_flushes_null_events_instead_of_the_batch() { + let (tx, rx) = broadcast::channel(4); + // Six changes into a capacity-4 channel: the receiver's first recv + // reports the two it missed. + for i in 0..6 { + tx.send(branch(&format!("b{i}"))).unwrap(); + } + drop(tx); + + let (emitted, sink) = collector(); + run(rx, sink).await; + + // Nothing from the four changes still buffered — just the flush. + assert_eq!(*emitted.lock().unwrap(), lag_flush_events().to_vec()); + } + + #[tokio::test(start_paused = true)] + async fn the_unlagged_path_still_dedupes_and_passes_changes_through() { + let (tx, rx) = broadcast::channel(16); + tx.send(branch("b1")).unwrap(); + tx.send(branch("b1")).unwrap(); + tx.send(StoreChange::Repos { github_repo: None }).unwrap(); + drop(tx); + + let (emitted, sink) = collector(); + run(rx, sink).await; + + assert_eq!( + *emitted.lock().unwrap(), + vec![ + ( + "branch-changed", + json!({ "branchId": "b1", "projectId": "p1" }) + ), + ("repos-changed", json!({ "githubRepo": null })), + ] + ); + } + + /// One instance of every [`StoreChange`] variant. + /// + /// The `match` below is the point: it has no wildcard arm, so adding a + /// variant to `StoreChange` fails compilation here until the new variant + /// is added to this list — which forces + /// [`the_lag_flush_covers_every_change_variants_event`] to be reconciled + /// with [`lag_flush_events`] at the same time. + fn one_of_each_change() -> [StoreChange; 5] { + let all = [ + StoreChange::Project { project_id: None }, + StoreChange::Branch { + branch_id: "b".to_string(), + project_id: None, + }, + StoreChange::Notes { + branch_id: None, + project_id: None, + }, + StoreChange::Review { + review_id: "r".to_string(), + branch_id: None, + }, + StoreChange::Repos { github_repo: None }, + ]; + for change in &all { + match change { + // New variant? Add it to `all` above and to `lag_flush_events`. + StoreChange::Project { .. } + | StoreChange::Branch { .. } + | StoreChange::Notes { .. } + | StoreChange::Review { .. } + | StoreChange::Repos { .. } => {} + } + } + all + } + + /// Pins `lag_flush_events` to `event_for`: the recovery flush must emit + /// exactly the event names a real mutation can emit, or the surface behind + /// the missing one silently loses lag recovery — the staleness the flush + /// exists to prevent. + #[test] + fn the_lag_flush_covers_every_change_variants_event() { + let flushed: std::collections::BTreeSet<&str> = + lag_flush_events().iter().map(|(event, _)| *event).collect(); + let emittable: std::collections::BTreeSet<&str> = one_of_each_change() + .iter() + .map(|change| event_for(change).0) + .collect(); + assert_eq!(flushed, emittable); + } + + #[tokio::test(start_paused = true)] + async fn lag_inside_the_window_discards_the_batch_accumulated_before_it() { + let (tx, rx) = broadcast::channel(4); + tx.send(branch("before-the-burst")).unwrap(); + + let (emitted, sink) = collector(); + let task = tokio::spawn(run(rx, sink)); + // Let the task take that first change and open the coalescing window. + tokio::task::yield_now().await; + + for i in 0..6 { + tx.send(branch(&format!("burst-{i}"))).unwrap(); + } + drop(tx); + task.await.unwrap(); + + assert_eq!(*emitted.lock().unwrap(), lag_flush_events().to_vec()); + } +} diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index 73b160159..67266d4c9 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -20,7 +20,6 @@ use std::collections::{HashMap, HashSet}; use std::path::Path; use std::sync::{Arc, Mutex, OnceLock}; use std::time::{SystemTime, UNIX_EPOCH}; -use tauri::Emitter; /// TTL for cached git user identity lookups (5 minutes). const GIT_USER_IDENTITY_TTL_MS: u128 = 300_000; @@ -734,7 +733,8 @@ pub(crate) fn refresh_branch_git_state_impl( }; if let Some(state) = git_state { - let _ = app.emit( + crate::web_server::emit_to_all( + app, "git-state-updated", GitStateUpdatedPayload { branch_id: branch_id.to_string(), diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index c0d633721..2c76aaafa 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -75,6 +75,24 @@ pub struct WebEvent { pub payload: String, } +const EVENT_GAP_EVENT: &str = "transport:event-gap"; + +fn serialize_web_event( + event_name: &str, + payload: S, +) -> Result { + serde_json::to_string(&serde_json::json!({ + "event": event_name, + "payload": payload, + })) +} + +fn event_gap_payload() -> String { + // serde_json::Value cannot fail serialization. + serialize_web_event(EVENT_GAP_EVENT, Value::Null) + .expect("event-gap payload serialization should be infallible") +} + // ============================================================================= // Event broadcast helper // ============================================================================= @@ -94,10 +112,7 @@ pub fn emit_to_all( use tauri::{Emitter, Manager}; let _ = app_handle.emit(event_name, payload.clone()); if let Some(tx) = app_handle.try_state::>() { - if let Ok(json) = serde_json::to_string(&serde_json::json!({ - "event": event_name, - "payload": payload, - })) { + if let Ok(json) = serialize_web_event(event_name, payload) { let _ = tx.send(WebEvent { event_name: event_name.to_string(), payload: json, @@ -299,13 +314,35 @@ async fn ws_events( Query(query): Query, State(state): State, ) -> Response { - let client_id = query - .client_id - .map(|id| id.trim().to_string()) - .filter(|id| !id.is_empty()); + let client_id = normalize_ws_client_id(query.client_id); ws.on_upgrade(move |socket| handle_ws(socket, state, client_id)) } +/// The scheduler client id a WS connection may claim, or `None` for "don't track +/// this socket in the PR-poll scheduler" — the socket still delivers events. +/// +/// Drops blank ids, and ids in the reserved native-window namespace (see +/// [`crate::pr_poll_scheduler::is_reserved_client_id`]): those are TTL-exempt, so +/// a web client holding one would pin its interest forever on a dirty drop, and +/// could spoof or tear down a real window's entry. Stripping rather than failing +/// the upgrade is deliberate — this socket also carries the change feed and +/// session events, so killing event delivery over a bad `clientId` would be the +/// worse failure mode. The warning is what makes a buggy client visible. +fn normalize_ws_client_id(client_id: Option) -> Option { + client_id + .map(|id| id.trim().to_string()) + .filter(|id| !id.is_empty()) + .filter(|id| { + let reserved = crate::pr_poll_scheduler::is_reserved_client_id(id); + if reserved { + log::warn!( + "[web_server] ignoring WS clientId {id:?}: the 'tauri-' prefix is reserved for native windows" + ); + } + !reserved + }) +} + // clippy's suggested fix (collapsing the inner `if` into a match guard) doesn't // compile because `data: Bytes` can't be moved out of the pattern binding into // the guard expression. @@ -331,6 +368,13 @@ async fn handle_ws(mut socket: WebSocket, state: WebAppState, client_id: Option< } Err(broadcast::error::RecvError::Lagged(n)) => { log::warn!("[web_server] WebSocket client lagged, dropped {n} events"); + if socket + .send(Message::Text(event_gap_payload().into())) + .await + .is_err() + { + break; + } } Err(broadcast::error::RecvError::Closed) => break, } @@ -427,6 +471,24 @@ fn opt_arg(args: &Value, key: &str) -> Result Result { + let client_id: String = arg(args, "clientId")?; + if crate::pr_poll_scheduler::is_reserved_client_id(&client_id) { + return Err(format!( + "clientId '{client_id}' uses the reserved native-window prefix 'tauri-'" + )); + } + Ok(client_id) +} + /// Helper to get the Store Arc from the shared mutex slot. fn get_store(store: &Mutex>>) -> Result, String> { store @@ -481,6 +543,18 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result Err("new_window is not supported in web mode".to_string()), + "take_window_seed" => { + // Web clients are never opener-seeded; report "no seed". + Ok(Value::Null) + } + "claim_updater_ownership" => { + Err("claim_updater_ownership is not supported in web mode".to_string()) + } + // ===================================================================== // Projects // ===================================================================== @@ -3586,19 +3660,19 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { - let client_id: String = arg(&args, "clientId")?; + let client_id = web_client_id(&args)?; let project_id: Option = opt_arg(&args, "projectId")?; pr_scheduler.set_foreground(client_id, project_id); Ok(Value::Null) } "set_focus" => { - let client_id: String = arg(&args, "clientId")?; + let client_id = web_client_id(&args)?; let focused: bool = arg(&args, "focused")?; pr_scheduler.set_focus(client_id, focused); Ok(Value::Null) } "set_branch_pending" => { - let client_id: String = arg(&args, "clientId")?; + let client_id = web_client_id(&args)?; let branch_id: String = arg(&args, "branchId")?; let project_id: String = arg(&args, "projectId")?; let pending: bool = arg(&args, "pending")?; @@ -3606,14 +3680,18 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { - let client_id: String = arg(&args, "clientId")?; + // Validated before either scheduler call: a rejected id must not + // half-apply (no `touch`, and no `force` either). + let client_id = web_client_id(&args)?; let project_id: String = arg(&args, "projectId")?; pr_scheduler.touch(client_id); pr_scheduler.force(project_id); Ok(Value::Null) } "disconnect_client" => { - let client_id: String = arg(&args, "clientId")?; + // Guarded too: this is the reverse-spoofing hole — a web caller + // tearing down a real native window's interest. + let client_id = web_client_id(&args)?; pr_scheduler.disconnect_client(client_id); Ok(Value::Null) } @@ -3723,8 +3801,61 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result(&event_gap_payload()).unwrap(), + json!({ + "event": "transport:event-gap", + "payload": null, + }) + ); + } + #[test] fn web_dispatch_covers_tauri_commands() { let tauri_commands = extract_generate_handler_commands(include_str!("lib.rs")); diff --git a/apps/staged/src-tauri/src/window_commands.rs b/apps/staged/src-tauri/src/window_commands.rs new file mode 100644 index 000000000..ab75673a6 --- /dev/null +++ b/apps/staged/src-tauri/src/window_commands.rs @@ -0,0 +1,353 @@ +//! Multi-window support: opening peer app windows. +//! +//! Every window is a full copy of the app — its own sidebar, navigation stack, +//! and selected project; there is no privileged "main" window beyond being the +//! one restored on cold start. New windows are built from the `main` window's +//! own `tauri.conf.json` entry, so the conf stays the single source of truth for +//! chrome — including `visible: false`, which lets the frontend show each window +//! once the theme is applied, exactly like the main window. + +use std::collections::{HashMap, HashSet}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Mutex; + +use tauri::Manager; + +/// Offset of each new window from the opener (focused) window, in logical px. +const CASCADE_OFFSET: f64 = 24.0; + +/// The currently focused window, if any. (`Manager::get_focused_window` is +/// behind tauri's `unstable` feature; this is its stable equivalent.) +/// +/// Generic over the runtime — the standard tauri-plugin idiom — so the tests +/// below can drive this module with tauri's `MockRuntime`. `generate_handler!` +/// and the menu handler both instantiate it with `Wry`, as before. +pub fn focused_window( + app: &tauri::AppHandle, +) -> Option> { + app.webview_windows() + .into_values() + .find(|w| w.is_focused().unwrap_or(false)) +} + +/// Managed state for window creation. +pub struct NewWindowState { + /// Suffix for the next `win-N` label. Starts at 2 (the first window is + /// `main`) and is never reused within a process, so two rapid `new_window` + /// calls cannot race to the same label. + next_index: AtomicUsize, + /// Project each not-yet-initialized window should open on, keyed by window + /// label. Written by [`open_new_window`], consumed once by + /// [`take_window_seed`], and cleared on window destruction for windows that + /// never initialize — or, when the window never got built at all, in + /// [`open_new_window`]'s own error arm. + seeds: Mutex>, +} + +impl NewWindowState { + pub fn new() -> Self { + Self { + next_index: AtomicUsize::new(2), + seeds: Mutex::new(HashMap::new()), + } + } + + /// Drop a window's unconsumed seed (called on window destruction). + pub fn discard_seed(&self, label: &str) { + self.seeds.lock().unwrap().remove(label); + } +} + +impl Default for NewWindowState { + fn default() -> Self { + Self::new() + } +} + +/// Process-wide ownership of the frontend updater loop. +/// +/// The updater UI still belongs to a webview, but exactly one live window may +/// check, prompt, and install at a time. Ownership is released by the native +/// `Destroyed` hook so a surviving peer can take over even when frontend +/// teardown does not run. +#[derive(Default)] +pub struct UpdaterWindowState { + inner: Mutex, +} + +#[derive(Default)] +struct UpdaterWindowInner { + owner: Option, + /// Window labels are never reused within a process. Remembering destroyed + /// ones rejects an IPC claim that was queued before destruction but did not + /// reach the backend until after the native hook ran. + destroyed: HashSet, +} + +impl UpdaterWindowState { + fn try_claim(&self, label: &str) -> bool { + let mut inner = self.inner.lock().unwrap(); + if inner.owner.is_some() || inner.destroyed.contains(label) { + return false; + } + inner.owner = Some(label.to_string()); + true + } + + /// Record native destruction and release ownership if `label` held it. + /// Returns whether peers should be notified that ownership is available. + pub fn window_destroyed(&self, label: &str) -> bool { + let mut inner = self.inner.lock().unwrap(); + inner.destroyed.insert(label.to_string()); + if inner.owner.as_deref() != Some(label) { + return false; + } + inner.owner = None; + true + } +} + +/// Open a new full-peer app window, optionally seeded with the opener's +/// selected project. Returns the new window's label. +/// +/// Labels are `win-N` to match the `win-*` glob in `capabilities/default.json`; +/// any other label would get no permissions, `invoke` would fail, and the +/// window would never show (the frontend only shows it after init). +/// +/// Callable directly (not just through the [`new_window`] command) so the menu +/// handler can open a window when no window is focused and there is therefore +/// no frontend to round-trip through. +pub fn open_new_window( + app: &tauri::AppHandle, + seed_project_id: Option, +) -> Result { + let state = app.state::(); + let label = format!("win-{}", state.next_index.fetch_add(1, Ordering::Relaxed)); + + // Cascade from the opener so the new window doesn't cover it exactly. The + // window-state plugin only restores the main window, so this position is + // not overridden. + let cascade = focused_window(app).and_then(|opener| { + let scale = opener.scale_factor().ok()?; + let position = opener.outer_position().ok()?.to_logical::(scale); + Some((position.x + CASCADE_OFFSET, position.y + CASCADE_OFFSET)) + }); + + // Build from the `main` window's own conf entry rather than restating its + // chrome here — one source of truth, so a conf tweak can't un-sync secondary + // windows. The entry has no explicit `label`, so it parses as "main". + let mut config = app + .config() + .app + .windows + .iter() + .find(|w| w.label == "main") + .ok_or("no `main` window entry in tauri.conf.json")? + .clone(); + config.label = label.clone(); + + let mut builder = tauri::WebviewWindowBuilder::from_config(app, &config) + .map_err(|e| format!("Failed to build window from config: {e}"))?; + + // The conf sets no `x`/`y` (the window-state plugin positions `main`), so the + // cascade is the only position setter — and it comes after `from_config`, so + // it would still win if the conf ever gained one. + if let Some((x, y)) = cascade { + builder = builder.position(x, y); + } + + // Seed before `build()`, not after: the consuming side (`take_window_seed`) + // can only be invoked from a webview with this label, and no such webview + // exists until `build()` creates it — so the seed is present before anyone + // can ask for it, without a lock spanning window creation. Everything + // fallible is deliberately above this line, leaving `build()` as the only + // error exit that has to clean up. + if let Some(project_id) = seed_project_id { + state + .seeds + .lock() + .unwrap() + .insert(label.clone(), project_id); + } + + let window = builder.build().map_err(|e| { + // No window with this label ever existed, so the `Destroyed` hook that + // normally discards an unconsumed seed can never fire for it — and + // labels are never reused within a process, so nothing else would ever + // remove the entry. Drop it here or it leaks until app exit. + state.discard_seed(&label); + format!("Failed to create window: {e}") + })?; + + Ok(window.label().to_string()) +} + +/// Frontend entry point for [`open_new_window`]. Stays `async` — the convention +/// for window-creating commands, which keeps them off wry's synchronous-command +/// path — but is await-free, so there is no cancellation point between +/// [`open_new_window`]'s seed insert and the cleanup in its error arm. +#[tauri::command] +pub async fn new_window( + app: tauri::AppHandle, + seed_project_id: Option, +) -> Result { + open_new_window(&app, seed_project_id) +} + +/// One-shot read of the project this window was seeded with by its opener. +/// Consumes the seed; only `win-*` windows ever have one. +#[tauri::command] +pub fn take_window_seed( + window: tauri::WebviewWindow, + state: tauri::State<'_, NewWindowState>, +) -> Option { + state.seeds.lock().unwrap().remove(window.label()) +} + +/// Atomically claim ownership of the app-wide updater loop for this window. +#[tauri::command] +pub fn claim_updater_ownership( + window: tauri::WebviewWindow, + state: tauri::State<'_, UpdaterWindowState>, +) -> bool { + state.try_claim(window.label()) +} + +#[cfg(test)] +mod tests { + use super::*; + use tauri::test::{mock_builder, mock_context, noop_assets}; + + /// A mock app with `windows` in its config — `mock_context` starts empty, so + /// tests that need [`open_new_window`]'s `main` lookup to succeed push a + /// default entry (whose `Default` label is `main`). + fn mock_app( + windows: Vec, + ) -> tauri::App { + let mut context = mock_context(noop_assets()); + context.config_mut().app.windows = windows; + let app = mock_builder() + .build(context) + .expect("failed to build mock app"); + app.manage(NewWindowState::new()); + app + } + + fn seeds_of(app: &tauri::App) -> HashMap { + app.state::().seeds.lock().unwrap().clone() + } + + #[test] + fn updater_ownership_transfers_only_after_the_owner_is_released() { + let state = UpdaterWindowState::default(); + + assert!(state.try_claim("main")); + assert!(!state.try_claim("win-2")); + assert!(!state.window_destroyed("win-2")); + assert!(!state.try_claim("win-2")); + + assert!(state.window_destroyed("main")); + assert!(!state.try_claim("main")); + assert!(state.try_claim("win-3")); + } + + /// The one fallible step after the seed insert must undo it: no window with + /// this label ever exists, so the `Destroyed` hook can't, and labels are + /// never reused, so nothing else ever will either. + #[test] + fn a_failed_build_discards_the_seed() { + let app = mock_app(vec![Default::default()]); + // Occupy the label the next `win-N` will mint, so `build()` fails the + // manager's duplicate-label check — which runs before any runtime call, + // making the failure deterministic on the mock runtime. + tauri::WebviewWindowBuilder::new(&app, "win-2", Default::default()) + .build() + .expect("failed to pre-create the colliding window"); + app.state::() + .seeds + .lock() + .unwrap() + .insert("win-99".into(), "other-project".into()); + + let result = open_new_window(app.handle(), Some("proj-1".into())); + + let error = result.expect_err("expected a duplicate-label build failure"); + assert!( + error.starts_with("Failed to create window"), + "expected the failure to come from `build()`, not an earlier step: {error}" + ); + assert_eq!( + seeds_of(&app), + HashMap::from([("win-99".to_string(), "other-project".to_string())]), + "the failed window's seed should be discarded, and only that one" + ); + } + + /// Pins the ordering: every fallible step other than `build()` sits above + /// the insert, so it exits with nothing to clean up. Moving the insert back + /// to the top of the function fails this. + #[test] + fn a_failure_before_the_build_never_inserts_a_seed() { + // No windows in the config, so the `main` lookup fails. + let app = mock_app(Vec::new()); + + let result = open_new_window(app.handle(), Some("proj-1".into())); + + assert!(result.is_err(), "expected the `main` config lookup to fail"); + assert!( + seeds_of(&app).is_empty(), + "no seed should have been inserted" + ); + } + + /// [`new_window`] clones the conf's `main` window entry. Giving that entry an + /// explicit non-`main` label would turn every New Window into a runtime error, so + /// catch it here instead. (An entry with no `label` parses as `main`.) + #[test] + fn conf_has_a_main_window_entry_for_new_window_to_clone() { + let conf: serde_json::Value = + serde_json::from_str(include_str!("../tauri.conf.json")).unwrap(); + let windows = conf["app"]["windows"] + .as_array() + .expect("tauri.conf.json has an app.windows array"); + + assert!( + windows + .iter() + .any(|w| w.get("label").is_none_or(|l| l == "main")), + "no window entry labelled `main` (explicitly or by default); \ + new_window has nothing to clone: {windows:?}" + ); + } + + /// Per-window titles (`windowTitle.ts`) call `setTitle`, which is *not* in + /// tauri's `core:window` default permission set — that set is getters only + /// (`allow-title` is there, `allow-set-title` is not). Dropping the grant + /// fails at runtime, not at build time: every title update rejects and the + /// Window menu silently keeps saying "Staged" in every window. The `win-*` + /// glob has to survive too, or only the first window gets titled. + #[test] + fn capabilities_grant_set_title_to_every_window() { + let capability: serde_json::Value = + serde_json::from_str(include_str!("../capabilities/default.json")).unwrap(); + + let permissions = capability["permissions"] + .as_array() + .expect("capabilities/default.json has a permissions array"); + assert!( + permissions + .iter() + .any(|p| p == "core:window:allow-set-title"), + "core:window:allow-set-title is missing; window titles will fail at \ + runtime: {permissions:?}" + ); + + let windows = capability["windows"] + .as_array() + .expect("capabilities/default.json has a windows array"); + assert!( + windows.iter().any(|w| w == "win-*"), + "the capability no longer covers secondary `win-N` windows: {windows:?}" + ); + } +} diff --git a/apps/staged/src/App.svelte b/apps/staged/src/App.svelte index ef7d682e2..4d680c14d 100644 --- a/apps/staged/src/App.svelte +++ b/apps/staged/src/App.svelte @@ -6,7 +6,13 @@ --> diff --git a/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte b/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte index c32756da0..acaad85ae 100644 --- a/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte +++ b/apps/staged/src/lib/features/projects/ProjectsSidebar.svelte @@ -285,8 +285,8 @@ const stopWatchingViewport = watchViewport(); void hydrateProjectsSidebarState(); - // Pin changes propagate through the store's staged:pinned-repos-changed - // listener; this mount only has to make sure the cache is warm. + // Pin changes propagate through the store's repos-changed listener; + // this mount only has to make sure the cache is warm. void projectsDataStore.ensureHomeReposLoaded(); return () => { diff --git a/apps/staged/src/lib/features/projects/RepoCard.svelte b/apps/staged/src/lib/features/projects/RepoCard.svelte index 9d296b0a6..3de9cb0a4 100644 --- a/apps/staged/src/lib/features/projects/RepoCard.svelte +++ b/apps/staged/src/lib/features/projects/RepoCard.svelte @@ -261,8 +261,9 @@ } else { await commands.pinRepo(repo.githubRepo, repo.subpath); } + // The pin write's repos-changed event refetches the home repo list in + // every window; onChange covers this view's immediate repaint. onChange?.(); - window.dispatchEvent(new CustomEvent('staged:pinned-repos-changed')); } catch (e) { console.error('[RepoCard] Failed to toggle pin:', e); toast.error('Failed to update pin', { diff --git a/apps/staged/src/lib/features/projects/projectActions.svelte.ts b/apps/staged/src/lib/features/projects/projectActions.svelte.ts index 736fb1775..34c0a0986 100644 --- a/apps/staged/src/lib/features/projects/projectActions.svelte.ts +++ b/apps/staged/src/lib/features/projects/projectActions.svelte.ts @@ -116,9 +116,12 @@ class ProjectActionsController { try { await commands.deleteProject(id); + // The delete's project-changed event prunes the project and its + // "Deleting…" marker from projectsDataStore, and each branch's + // branch-changed invalidates its timeline caches. In web mode an echo + // lost to a WebSocket gap is recovered by the reconnect revalidation in + // transport.ts, so the marker can't outlive the socket outage. projectStateStore.markAsRead(id); - projectsDataStore.projectDeleteFinished(id, { removed: true }); - commands.invalidateProjectBranchTimelines(branchesToClear.map((b) => b.id)); for (const branch of branchesToClear) { workspaceLifecycle.clearBranchState(branch.id); } @@ -126,7 +129,7 @@ class ProjectActionsController { console.error('Failed to delete project:', e); const message = e instanceof Error ? e.message : String(e); toast.error('Unable to delete project', { description: message }); - projectsDataStore.projectDeleteFinished(id); + projectsDataStore.projectDeleteFailed(id); } } } diff --git a/apps/staged/src/lib/features/projects/projectActions.test.ts b/apps/staged/src/lib/features/projects/projectActions.test.ts index 423d7ca9f..f8c00eaad 100644 --- a/apps/staged/src/lib/features/projects/projectActions.test.ts +++ b/apps/staged/src/lib/features/projects/projectActions.test.ts @@ -70,7 +70,6 @@ function projectRepo(overrides: Partial = {}): ProjectRepo { let deleteProject: ReturnType; let hasUnpushedCommits: ReturnType; -let invalidateProjectBranchTimelines: ReturnType; let markAsRead: ReturnType; let markAsUnread: ReturnType; let clearBranchState: ReturnType; @@ -79,7 +78,7 @@ let selectProject: ReturnType; let goHome: ReturnType; let navigationState: { selectedProjectId: string | null }; let projectDeleteStarted: ReturnType; -let projectDeleteFinished: ReturnType; +let projectDeleteFailed: ReturnType; let ensureProjectHydrated: ReturnType; /** Mutable backing state for the mocked projectsDataStore. */ @@ -103,7 +102,6 @@ beforeEach(() => { deleteProject = vi.fn().mockResolvedValue(undefined); hasUnpushedCommits = vi.fn().mockResolvedValue(false); - invalidateProjectBranchTimelines = vi.fn(); markAsRead = vi.fn(); markAsUnread = vi.fn(); clearBranchState = vi.fn(); @@ -112,7 +110,7 @@ beforeEach(() => { goHome = vi.fn(); navigationState = { selectedProjectId: null }; projectDeleteStarted = vi.fn(); - projectDeleteFinished = vi.fn(); + projectDeleteFailed = vi.fn(); // Default: the project is already hydrated, so ensuring it is a no-op. ensureProjectHydrated = vi.fn().mockResolvedValue(undefined); @@ -126,7 +124,6 @@ beforeEach(() => { vi.doMock('../../api/commands', () => ({ deleteProject, hasUnpushedCommits, - invalidateProjectBranchTimelines, })); vi.doMock('../layout/navigation.svelte', () => ({ navigation: navigationState, @@ -157,7 +154,7 @@ beforeEach(() => { isProjectDeleting: (projectId: string) => storeState.deletingProjectNames.has(projectId), ensureProjectHydrated, projectDeleteStarted, - projectDeleteFinished, + projectDeleteFailed, }, })); vi.doMock('../../stores/projectState.svelte', () => ({ @@ -212,8 +209,9 @@ describe('requestRemoveProject', () => { expect(projectDeleteStarted).toHaveBeenCalledWith('p1', 'Alpha'); expect(deleteProject).toHaveBeenCalledWith('p1'); expect(markAsRead).toHaveBeenCalledWith('p1'); - expect(projectDeleteFinished).toHaveBeenCalledWith('p1', { removed: true }); - expect(invalidateProjectBranchTimelines).toHaveBeenCalledWith(['b1']); + // Success leaves the deleting marker alone: the delete's project-changed + // refetch prunes the project and the marker together. + expect(projectDeleteFailed).not.toHaveBeenCalled(); expect(clearBranchState).toHaveBeenCalledWith('b1'); }); @@ -247,7 +245,7 @@ describe('requestRemoveProject', () => { expect(toastError).toHaveBeenCalledWith('Unable to delete project', { description: 'backend down', }); - expect(projectDeleteFinished).toHaveBeenCalledWith('p1'); + expect(projectDeleteFailed).toHaveBeenCalledWith('p1'); expect(markAsRead).not.toHaveBeenCalled(); expect(clearBranchState).not.toHaveBeenCalled(); }); @@ -318,7 +316,6 @@ describe('hydration before the safety check', () => { await actions.requestRemoveProject(p); expect(deleteProject).toHaveBeenCalledWith('p1'); - expect(invalidateProjectBranchTimelines).toHaveBeenCalledWith(['b1', 'b2']); expect(clearBranchState).toHaveBeenCalledWith('b1'); expect(clearBranchState).toHaveBeenCalledWith('b2'); }); @@ -352,7 +349,7 @@ describe('confirmation dialog flow', () => { expect(actions.pendingDelete).toBeNull(); expect(deleteProject).toHaveBeenCalledWith('p1'); - expect(projectDeleteFinished).toHaveBeenCalledWith('p1', { removed: true }); + expect(projectDeleteFailed).not.toHaveBeenCalled(); }); it('cancelPendingDelete dismisses without deleting', async () => { diff --git a/apps/staged/src/lib/features/projects/workspaceLifecycle.svelte.ts b/apps/staged/src/lib/features/projects/workspaceLifecycle.svelte.ts index ccbbde7b2..014d2150f 100644 --- a/apps/staged/src/lib/features/projects/workspaceLifecycle.svelte.ts +++ b/apps/staged/src/lib/features/projects/workspaceLifecycle.svelte.ts @@ -515,7 +515,6 @@ class WorkspaceLifecycleController { ) ); } - commands.invalidateBranchTimeline(branchId); // NOTE: prerun actions are only triggered here when opts.runPrerun // is set (e.g. the retry path). The normal creation paths run them diff --git a/apps/staged/src/lib/features/projects/workspaceLifecycle.test.ts b/apps/staged/src/lib/features/projects/workspaceLifecycle.test.ts index d7ddf8744..21a7087e8 100644 --- a/apps/staged/src/lib/features/projects/workspaceLifecycle.test.ts +++ b/apps/staged/src/lib/features/projects/workspaceLifecycle.test.ts @@ -15,7 +15,6 @@ describe('WorkspaceLifecycleController.retryWorktree', () => { vi.doMock('../../api/commands', () => ({ setupWorktree: vi.fn(), setupWorktreeAndRunPrerun, - invalidateBranchTimeline: vi.fn(), drainQueuedSessions: vi.fn(async () => {}), pollAllWorkspaceStatuses: vi.fn(async () => ({})), })); diff --git a/apps/staged/src/lib/listeners/cacheInvalidationListener.test.ts b/apps/staged/src/lib/listeners/cacheInvalidationListener.test.ts new file mode 100644 index 000000000..136d14f78 --- /dev/null +++ b/apps/staged/src/lib/listeners/cacheInvalidationListener.test.ts @@ -0,0 +1,136 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { BranchChangedEvent, ProjectChangedEvent } from '../types'; + +/** + * The project-changed and branch-changed legs of the cache listener, + * including the store change feed's lag recovery: a null id means the feed + * dropped changes it can no longer name, so invalidation widens from one + * project/branch to all of them. + */ + +type EventCallback = (payload: unknown) => void; + +let eventCallbacks: Map; +let invalidateCache: ReturnType; +let invalidateCacheByArgs: ReturnType; +let invalidateCacheByCommand: ReturnType; +let invalidateBranchTimeline: ReturnType; +let invalidateAllBranchTimelines: ReturnType; + +function emitProjectChanged(payload: ProjectChangedEvent): void { + eventCallbacks.get('project-changed')?.(payload); +} + +function emitBranchChanged(payload: BranchChangedEvent): void { + eventCallbacks.get('branch-changed')?.(payload); +} + +async function startListening() { + const { listenForCacheInvalidation } = await import('./cacheInvalidationListener'); + return listenForCacheInvalidation(); +} + +beforeEach(() => { + vi.resetModules(); + eventCallbacks = new Map(); + invalidateCache = vi.fn(); + invalidateCacheByArgs = vi.fn(); + invalidateCacheByCommand = vi.fn(); + invalidateBranchTimeline = vi.fn(); + invalidateAllBranchTimelines = vi.fn(); + + vi.doMock('../transport', () => ({ + listenToEvent: (event: string, callback: EventCallback) => { + eventCallbacks.set(event, callback); + return () => eventCallbacks.delete(event); + }, + })); + vi.doMock('../cache', () => ({ + invalidateCache, + invalidateCacheByArgs, + invalidateCacheByCommand, + })); + vi.doMock('../commands', () => ({ + invalidateBranchTimeline, + invalidateAllBranchTimelines, + })); +}); + +afterEach(() => { + vi.doUnmock('../transport'); + vi.doUnmock('../cache'); + vi.doUnmock('../commands'); +}); + +describe('project-changed cache invalidation', () => { + it('drops the project list and just the named project’s repo list', async () => { + await startListening(); + + emitProjectChanged({ projectId: 'p1' }); + + // list_projects takes no args, so the drop is command-wide either way. + expect(invalidateCacheByCommand.mock.calls).toEqual([['list_projects']]); + expect(invalidateCacheByArgs.mock.calls).toEqual([['list_project_repos', { projectId: 'p1' }]]); + }); + + it('widens the repo-list drop when the lag flush names no project', async () => { + await startListening(); + + emitProjectChanged({ projectId: null }); + + expect(invalidateCacheByCommand.mock.calls).toEqual([ + ['list_projects'], + ['list_project_repos'], + ]); + expect(invalidateCacheByArgs).not.toHaveBeenCalled(); + }); +}); + +describe('branch-changed cache invalidation', () => { + it('invalidates just the named branch’s timeline and diffs, and just the named project’s list', async () => { + await startListening(); + + emitBranchChanged({ branchId: 'b1', projectId: 'p1' }); + + expect(invalidateBranchTimeline).toHaveBeenCalledWith('b1'); + expect(invalidateAllBranchTimelines).not.toHaveBeenCalled(); + expect(invalidateCacheByArgs).toHaveBeenCalledWith('list_branches_for_project', { + projectId: 'p1', + }); + expect(invalidateCacheByArgs).toHaveBeenCalledWith('get_diff_files', { branchId: 'b1' }); + expect(invalidateCacheByArgs).toHaveBeenCalledWith('get_file_diff', { branchId: 'b1' }); + // Nothing widens: another project's cached branch list keeps its instant paint. + expect(invalidateCacheByCommand).not.toHaveBeenCalled(); + }); + + it('widens the branch-list drop when the backend couldn’t resolve the project', async () => { + await startListening(); + + emitBranchChanged({ branchId: 'b1', projectId: null }); + + expect(invalidateCacheByCommand.mock.calls).toEqual([['list_branches_for_project']]); + // The branch-scoped half is unaffected by an unresolved project. + expect(invalidateBranchTimeline).toHaveBeenCalledWith('b1'); + expect(invalidateAllBranchTimelines).not.toHaveBeenCalled(); + expect(invalidateCacheByArgs.mock.calls).toEqual([ + ['get_diff_files', { branchId: 'b1' }], + ['get_file_diff', { branchId: 'b1' }], + ]); + }); + + it('widens to every branch when the lag flush names none', async () => { + await startListening(); + + emitBranchChanged({ branchId: null, projectId: null }); + + expect(invalidateAllBranchTimelines).toHaveBeenCalledTimes(1); + expect(invalidateBranchTimeline).not.toHaveBeenCalled(); + // Command-wide, since there's no branch id to match cached args against. + expect(invalidateCacheByArgs).not.toHaveBeenCalled(); + expect(invalidateCacheByCommand.mock.calls.map(([command]) => command)).toEqual([ + 'list_branches_for_project', + 'get_diff_files', + 'get_file_diff', + ]); + }); +}); diff --git a/apps/staged/src/lib/listeners/cacheInvalidationListener.ts b/apps/staged/src/lib/listeners/cacheInvalidationListener.ts index 920f08da5..14071e1b5 100644 --- a/apps/staged/src/lib/listeners/cacheInvalidationListener.ts +++ b/apps/staged/src/lib/listeners/cacheInvalidationListener.ts @@ -1,19 +1,35 @@ /** * Event-driven cache invalidation listener. * - * Listens for backend events (pr-status-changed, branch-git-state-changed) - * and invalidates the corresponding IndexedDB cache entries so that stale - * data is never served after the backend pushes an update. + * Listens for backend events (pr-status-changed and the store change feed's + * project-changed / branch-changed / notes-changed / review-changed) and + * invalidates the corresponding caches so that stale data is never served + * after the backend pushes an update. The store change feed publishes from + * every mutating store method, so a write in any window (or the backend + * itself) invalidates every window. + * + * This is the cache leg only. The in-memory view stores subscribe to the + * same feed themselves: projectsDataStore consumes project-changed, + * branch-changed, and repos-changed for the project/branch/repo lists. Its + * event-driven refetches bypass cache reads, but this listener still protects + * shared IDB data for the next non-forced read in this tab or another tab. + * + * When the feed falls behind it emits every event with all ids null, which + * these handlers read as "refetch the whole surface" — one broad + * invalidation instead of silently stale windows. */ import { listenToEvent, type UnlistenFn } from '../transport'; import { invalidateCache, invalidateCacheByArgs, invalidateCacheByCommand } from '../cache'; -import { invalidateBranchTimeline } from '../commands'; -import type { PrStatusChangedEvent, SessionStatusPayload } from '../types'; - -interface BranchGitStateChangedEvent { - branchId: string; -} +import { invalidateAllBranchTimelines, invalidateBranchTimeline } from '../commands'; +import type { + BranchChangedEvent, + NotesChangedEvent, + PrStatusChangedEvent, + ProjectChangedEvent, + ReviewChangedEvent, + SessionStatusPayload, +} from '../types'; export function listenForCacheInvalidation(): UnlistenFn { const unlisteners: UnlistenFn[] = []; @@ -25,16 +41,78 @@ export function listenForCacheInvalidation(): UnlistenFn { }) ); - // Branch git state changed → invalidate timeline and diff caches + // Project changed (create / rename / delete / repo attach) → drop the + // project-list caches for future non-forced reads in this tab and any other + // tab sharing the same IDB store. list_projects takes no args so the drop is + // command-wide; the repo lists scope to the named project when the payload + // names one, widening only on the feed's lag recovery. unlisteners.push( - listenToEvent('branch-git-state-changed', (payload) => { + listenToEvent('project-changed', (payload) => { + invalidateCacheByCommand('list_projects'); + if (payload.projectId === null) { + invalidateCacheByCommand('list_project_repos'); + } else { + invalidateCacheByArgs('list_project_repos', { projectId: payload.projectId }); + } + }) + ); + + // Branch changed (any store write touching the branch or its timeline) + // → invalidate that branch's list, timeline and diff caches. Three tiers, + // widening as the payload names less: + // - a named project scopes the branch-list drop to that one project, so + // every other project keeps its instant cached paint. A mutation + // touching two projects' lists (a move) publishes once per project, + // so scoping loses nothing; + // - an unresolved project widens to every project's list, matching the + // store's own scan-all-known-lists fallback; + // - a null branchId is the feed's lag recovery — it dropped changes it + // can't name, so widen to every branch as well. + unlisteners.push( + listenToEvent('branch-changed', (payload) => { + if (payload.branchId === null) { + invalidateCacheByCommand('list_branches_for_project'); + invalidateAllBranchTimelines(); + invalidateCacheByCommand('get_diff_files'); + invalidateCacheByCommand('get_file_diff'); + return; + } + if (payload.projectId === null) { + invalidateCacheByCommand('list_branches_for_project'); + } else { + invalidateCacheByArgs('list_branches_for_project', { projectId: payload.projectId }); + } invalidateBranchTimeline(payload.branchId); - invalidateCacheByCommand('list_branches_for_project'); invalidateCacheByArgs('get_diff_files', { branchId: payload.branchId }); invalidateCacheByArgs('get_file_diff', { branchId: payload.branchId }); }) ); + // Notes changed → branch notes are timeline items, so refresh that branch's + // timeline; project-note surfaces (ProjectSection's list, BranchCard's + // hashtag items) refetch through the existing window event. + unlisteners.push( + listenToEvent('notes-changed', (payload) => { + if (payload.branchId) { + invalidateBranchTimeline(payload.branchId); + } else { + window.dispatchEvent(new CustomEvent('project-notes-invalidated')); + } + }) + ); + + // Review changed → reviews and their comment counts render as timeline + // items. An open diff viewer keeps its own optimistic review state and is + // deliberately not reloaded here: the echo of a window's own edit would + // clobber in-flight comment drafts. + unlisteners.push( + listenToEvent('review-changed', (payload) => { + if (payload.branchId) { + invalidateBranchTimeline(payload.branchId); + } + }) + ); + // Session status changed → invalidate cached session messages when a session // completes, errors, or is cancelled (messages are now final) unlisteners.push( diff --git a/apps/staged/src/lib/listeners/menuListener.test.ts b/apps/staged/src/lib/listeners/menuListener.test.ts new file mode 100644 index 000000000..ea7eacdb5 --- /dev/null +++ b/apps/staged/src/lib/listeners/menuListener.test.ts @@ -0,0 +1,201 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * The app-menu listener. The properties worth pinning are structural: exactly + * the nine backend menu events are registered, they're window-scoped (the + * backend emits to the focused window, so an any-target listener would fire in + * every window), and every one of them is torn down — the omission this module + * exists to make impossible. + */ + +type EventCallback = (payload: unknown) => void; + +/** Mirrors `MENU_ITEMS` in `lib.rs`, hard-coded rather than read from the module. */ +const MENU_EVENTS = [ + 'menu:new-window', + 'menu:settings', + 'menu:find', + 'menu:find-next', + 'menu:find-previous', + 'menu:delete-project', + 'menu:zoom-in', + 'menu:zoom-out', + 'menu:zoom-reset', +]; + +let windowCallbacks: Map; +let unlistenSpies: Map>; +let listenToEvent: ReturnType; +let newWindow: ReturnType; +let triggerShortcut: ReturnType; +let runSearchShortcut: ReturnType; +let openSettings: ReturnType; +let increaseSize: ReturnType; +let decreaseSize: ReturnType; +let resetSize: ReturnType; +let navigation: { selectedProjectId: string | null }; + +async function startListening() { + const { listenForMenuEvents } = await import('./menuListener'); + return listenForMenuEvents(); +} + +function fire(event: string): void { + const callback = windowCallbacks.get(event); + if (!callback) throw new Error(`no listener registered for ${event}`); + callback(undefined); +} + +beforeEach(() => { + vi.resetModules(); + windowCallbacks = new Map(); + unlistenSpies = new Map(); + listenToEvent = vi.fn(); + newWindow = vi.fn(() => Promise.resolve('win-2')); + triggerShortcut = vi.fn(() => false); + runSearchShortcut = vi.fn(() => true); + openSettings = vi.fn(); + increaseSize = vi.fn(); + decreaseSize = vi.fn(); + resetSize = vi.fn(); + navigation = { selectedProjectId: null }; + + vi.doMock('../transport', () => ({ + listenToEvent, + listenToWindowEvent: (event: string, callback: EventCallback) => { + windowCallbacks.set(event, callback); + const unlisten = vi.fn(() => windowCallbacks.delete(event)); + unlistenSpies.set(event, unlisten); + return unlisten; + }, + })); + vi.doMock('../commands', () => ({ newWindow })); + vi.doMock('../features/layout/navigation.svelte', () => ({ + get navigation() { + return navigation; + }, + openSettings, + })); + vi.doMock('../features/keyboard/shortcuts', () => ({ triggerShortcut })); + vi.doMock('../features/keyboard/searchTargets', () => ({ runSearchShortcut })); + vi.doMock('../features/settings/preferences.svelte', () => ({ + increaseSize, + decreaseSize, + resetSize, + })); +}); + +afterEach(() => { + vi.doUnmock('../transport'); + vi.doUnmock('../commands'); + vi.doUnmock('../features/layout/navigation.svelte'); + vi.doUnmock('../features/keyboard/shortcuts'); + vi.doUnmock('../features/keyboard/searchTargets'); + vi.doUnmock('../features/settings/preferences.svelte'); +}); + +describe('listenForMenuEvents', () => { + it('registers exactly the nine menu events, window-scoped', async () => { + await startListening(); + + expect([...windowCallbacks.keys()]).toEqual(MENU_EVENTS); + // Any-target listeners would also match emits addressed to other windows. + expect(listenToEvent).not.toHaveBeenCalled(); + }); + + it('tears down every listener it registered', async () => { + const unlistenMenu = await startListening(); + + unlistenMenu(); + + expect(unlistenSpies.size).toBe(MENU_EVENTS.length); + for (const [event, unlisten] of unlistenSpies) { + expect(unlisten, `${event} should be unlistened exactly once`).toHaveBeenCalledTimes(1); + } + expect(windowCallbacks.size).toBe(0); + }); + + it('opens a new window seeded with the opener’s selected project', async () => { + navigation.selectedProjectId = 'p1'; + await startListening(); + + fire('menu:new-window'); + + expect(newWindow).toHaveBeenCalledWith('p1'); + }); + + it('passes a null seed when no project is selected, and swallows failures', async () => { + newWindow.mockImplementation(() => Promise.reject(new Error('build failed'))); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + await startListening(); + + fire('menu:new-window'); + await Promise.resolve(); + + expect(newWindow).toHaveBeenCalledWith(null); + // Caught, so it can't escape as an unhandled rejection. + expect(consoleError).toHaveBeenCalledTimes(1); + consoleError.mockRestore(); + }); + + it('gives a registered shortcut first refusal before the app-level fallback', async () => { + await startListening(); + + const cases: { + event: string; + shortcut: string; + fallback: () => ReturnType; + arg?: string; + }[] = [ + { event: 'menu:settings', shortcut: 'app-open-settings', fallback: () => openSettings }, + { + event: 'menu:find', + shortcut: 'search-find', + fallback: () => runSearchShortcut, + arg: 'find', + }, + { + event: 'menu:find-next', + shortcut: 'search-find-next', + fallback: () => runSearchShortcut, + arg: 'next', + }, + { + event: 'menu:find-previous', + shortcut: 'search-find-previous', + fallback: () => runSearchShortcut, + arg: 'previous', + }, + { event: 'menu:zoom-in', shortcut: 'view-increase-size', fallback: () => increaseSize }, + { event: 'menu:zoom-out', shortcut: 'view-decrease-size', fallback: () => decreaseSize }, + { event: 'menu:zoom-reset', shortcut: 'view-reset-size', fallback: () => resetSize }, + ]; + const allFallbacks = [openSettings, runSearchShortcut, increaseSize, decreaseSize, resetSize]; + + for (const { event, shortcut, fallback, arg } of cases) { + // Claimed by the focused surface → no fallback runs. + vi.clearAllMocks(); + triggerShortcut.mockReturnValue(true); + fire(event); + expect(triggerShortcut, event).toHaveBeenCalledExactlyOnceWith(shortcut); + for (const spy of allFallbacks) expect(spy, event).not.toHaveBeenCalled(); + + // Unclaimed → the app-level action runs. + vi.clearAllMocks(); + triggerShortcut.mockReturnValue(false); + fire(event); + const expected = arg === undefined ? [] : [arg]; + expect(fallback(), event).toHaveBeenCalledExactlyOnceWith(...expected); + } + }); + + it('routes delete-project to its shortcut with no fallback', async () => { + await startListening(); + + fire('menu:delete-project'); + + expect(triggerShortcut).toHaveBeenCalledExactlyOnceWith('app-delete-project'); + expect(openSettings).not.toHaveBeenCalled(); + expect(runSearchShortcut).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/staged/src/lib/listeners/menuListener.ts b/apps/staged/src/lib/listeners/menuListener.ts new file mode 100644 index 000000000..11367eda7 --- /dev/null +++ b/apps/staged/src/lib/listeners/menuListener.ts @@ -0,0 +1,74 @@ +/** + * App-menu listener. + * + * The macOS app menu is owned by the backend, which routes each item to the + * focused window with `emit_to` (`dispatch_menu_event` in `lib.rs`). That is + * why these must be *window-scoped* listeners: `listenToEvent` registers an + * any-target listener, which Tauri also matches against emits addressed to + * other windows, so one Cmd+, would open settings in every window. With no + * window focused (every window minimized — the macOS menu stays live) nothing + * arrives here at all: the backend drops the window-scoped items and creates a + * New Window itself, unseeded. + * + * Registration and teardown are derived from the same table, so an item can't + * be registered without also being torn down. + * + * The listeners are registered unconditionally. In web mode they fall through + * to the shared WebSocket, and the web server never emits `menu:*`, so they + * are inert. + */ + +import { listenToWindowEvent, type UnlistenFn } from '../transport'; +import { newWindow } from '../commands'; +import { navigation, openSettings } from '../features/layout/navigation.svelte'; +import { triggerShortcut } from '../features/keyboard/shortcuts'; +import { runSearchShortcut } from '../features/keyboard/searchTargets'; +import { increaseSize, decreaseSize, resetSize } from '../features/settings/preferences.svelte'; + +/** + * Menu event → handler, mirroring the backend's menu-id → event table. Most + * items offer a registered shortcut first refusal so the focused surface can + * claim it (a modal's own Find, say), falling back to the app-level action. + */ +const handlers: Record void> = { + 'menu:new-window': () => { + // The new window inherits this window's selected project. + void newWindow(navigation.selectedProjectId ?? null).catch((e) => { + console.error('Failed to open new window:', e); + }); + }, + 'menu:settings': () => { + if (!triggerShortcut('app-open-settings')) openSettings(); + }, + 'menu:find': () => { + if (!triggerShortcut('search-find')) runSearchShortcut('find'); + }, + 'menu:find-next': () => { + if (!triggerShortcut('search-find-next')) runSearchShortcut('next'); + }, + 'menu:find-previous': () => { + if (!triggerShortcut('search-find-previous')) runSearchShortcut('previous'); + }, + 'menu:delete-project': () => { + triggerShortcut('app-delete-project'); + }, + 'menu:zoom-in': () => { + if (!triggerShortcut('view-increase-size')) increaseSize(); + }, + 'menu:zoom-out': () => { + if (!triggerShortcut('view-decrease-size')) decreaseSize(); + }, + 'menu:zoom-reset': () => { + if (!triggerShortcut('view-reset-size')) resetSize(); + }, +}; + +export function listenForMenuEvents(): UnlistenFn { + const unlisteners = Object.entries(handlers).map(([event, handler]) => + listenToWindowEvent(event, handler) + ); + + return () => { + for (const unlisten of unlisteners) unlisten(); + }; +} diff --git a/apps/staged/src/lib/listeners/pageLifecycleListener.test.ts b/apps/staged/src/lib/listeners/pageLifecycleListener.test.ts index 8fddf30df..16ca640c6 100644 --- a/apps/staged/src/lib/listeners/pageLifecycleListener.test.ts +++ b/apps/staged/src/lib/listeners/pageLifecycleListener.test.ts @@ -25,15 +25,22 @@ import { describe('pageLifecycleListener', () => { let unlisten: () => void; let cacheStaleEvents: Event[]; + let projectNotesEvents: Event[]; function onCacheStale(e: Event) { cacheStaleEvents.push(e); } + function onProjectNotesInvalidated(e: Event) { + projectNotesEvents.push(e); + } + beforeEach(() => { mockMarkAllStale.mockClear(); cacheStaleEvents = []; + projectNotesEvents = []; window.addEventListener('cache-stale', onCacheStale); + window.addEventListener('project-notes-invalidated', onProjectNotesInvalidated); _setLastActivityTimestamp(Date.now()); // Reset the hide timestamp so each test starts from "no recorded hide", // which (safely) revalidates on resume. @@ -44,6 +51,7 @@ describe('pageLifecycleListener', () => { afterEach(() => { unlisten(); window.removeEventListener('cache-stale', onCacheStale); + window.removeEventListener('project-notes-invalidated', onProjectNotesInvalidated); }); describe('resume event', () => { @@ -55,6 +63,8 @@ describe('pageLifecycleListener', () => { expect(mockMarkAllStale).toHaveBeenCalledTimes(1); }); expect(cacheStaleEvents).toHaveLength(1); + // Project notes have no cache-stale consumer of their own. + expect(projectNotesEvents).toHaveLength(1); }); it('updates lastActivityTimestamp after resume', async () => { diff --git a/apps/staged/src/lib/listeners/pageLifecycleListener.ts b/apps/staged/src/lib/listeners/pageLifecycleListener.ts index 0efa251be..5aa48010c 100644 --- a/apps/staged/src/lib/listeners/pageLifecycleListener.ts +++ b/apps/staged/src/lib/listeners/pageLifecycleListener.ts @@ -14,6 +14,12 @@ * Going hidden also persists a synchronous snapshot of the in-memory timeline * cache (see commands.ts) — iOS tears the tab down while hidden, so this is the * last chance to capture state for the next cold boot's first-frame paint. + * + * The revalidation itself (`revalidateAll`) is exported, because page resume is + * no longer its only trigger: the web-mode event socket calls it on reconnect + * (transport.ts), where every event emitted during the socket gap is lost for + * good. It is the shared "assume everything since the last known-good point was + * missed" recovery — the gating differs per caller, the recovery doesn't. */ import { isTauri } from '../transport'; @@ -39,9 +45,27 @@ function shouldRevalidateAfterResume(): boolean { return hiddenAt === 0 || Date.now() - hiddenAt > RESUME_REVALIDATE_THRESHOLD_MS; } -async function revalidateAll() { +/** + * Recover from an unknown-length gap in event delivery: every cached entry is + * marked stale so the next read revalidates over the network, and mounted views + * refetch immediately. + * + * Callers: page resume (gated on the hidden duration, below) and web-mode + * WebSocket reconnect (transport.ts), where the store change feed, PR-poll and + * session events emitted while the socket was down are unrecoverable — the + * server keeps no per-client queue. + * + * `markAllStale()` must precede the `cache-stale` dispatch: an unmarked SWR hit + * would serve a cached list with no revalidating leg, so the handlers would + * refetch nothing. + */ +export async function revalidateAll() { await markAllStale(); window.dispatchEvent(new CustomEvent('cache-stale')); + // Project notes are the one surface with no `cache-stale` consumer: they + // refetch only on this event, so a note change missed during the gap would + // stay invisible until the next notes-changed. + window.dispatchEvent(new CustomEvent('project-notes-invalidated')); } /** Record the page going hidden and snapshot caches for the next cold boot. */ diff --git a/apps/staged/src/lib/services/prPollingService.ts b/apps/staged/src/lib/services/prPollingService.ts index f19679811..060d42c94 100644 --- a/apps/staged/src/lib/services/prPollingService.ts +++ b/apps/staged/src/lib/services/prPollingService.ts @@ -17,7 +17,7 @@ * `pr-statuses-refreshed`; components subscribe to those directly. */ -import { isTauri, listenToEvent, type UnlistenFn } from '../transport'; +import { getWindowLabel, isTauri, listenToEvent, type UnlistenFn } from '../transport'; import { setForegroundProject, setPrPollFocus, @@ -35,17 +35,19 @@ import { // owns a stable id for the lifetime of the page that is threaded through every // interest hint. // -// - Native (Tauri): the fixed well-known id `tauri-main` (must match -// `TAURI_CLIENT_ID` in pr_poll_scheduler.rs), so single-client behaviour is -// identical to before per-client interest existed. +// - Native (Tauri): `tauri-` — one client per window, so each +// window's project selection and focus count independently in the +// backend's union (the first window's label is `main`, keeping its id +// byte-for-byte the pre-seeded `tauri-main`). The `tauri-` prefix must +// match `TAURI_CLIENT_PREFIX` in pr_poll_scheduler.rs: it exempts native +// windows from TTL eviction (they have no WS heartbeat); the backend drops +// the client when the window is destroyed. // - Web (browser): a fresh UUID per page load (per tab). The same value must // be appended to the WS connect URL (`?clientId=`) by the web transport // so the backend correlates this client's interest (invoke channel) with // its disconnect (WS close). See `getPrPollClientId`. -const TAURI_CLIENT_ID = 'tauri-main'; - -const clientId: string = isTauri ? TAURI_CLIENT_ID : crypto.randomUUID(); +const clientId: string = isTauri ? `tauri-${getWindowLabel() ?? 'main'}` : crypto.randomUUID(); /** * This client's PR-poll id, stable for the page's lifetime. Exposed so the web diff --git a/apps/staged/src/lib/shared/RepoLabel.svelte b/apps/staged/src/lib/shared/RepoLabel.svelte index 445763146..5ffcb7099 100644 --- a/apps/staged/src/lib/shared/RepoLabel.svelte +++ b/apps/staged/src/lib/shared/RepoLabel.svelte @@ -10,6 +10,8 @@ githubRepo="block/mark" subpath="ui" → "block/mark/" muted + "ui" primary --> diff --git a/apps/staged/src/lib/shared/repoLabel.ts b/apps/staged/src/lib/shared/repoLabel.ts new file mode 100644 index 000000000..82de3e291 --- /dev/null +++ b/apps/staged/src/lib/shared/repoLabel.ts @@ -0,0 +1,25 @@ +/** + * The distinguishing part of a repo path. + * + * `RepoLabel.svelte` renders this segment at full contrast with everything + * before it muted; the per-window title (`features/layout/windowTitle.ts`) + * renders it *alone*, since macOS Window-menu items truncate and + * `block/builderbot/apps/staged` would be mostly wasted width. Shared so the + * chip and the title can't disagree about which segment matters. + */ + +export interface RepoPathRef { + repo: string; + subpath?: string | null; +} + +/** + * The subpath when there is one, otherwise the text after the repo's final + * `/`. A multi-segment subpath stays whole (`apps/staged`, not `staged`) — the + * subpath is one unit, and this is the rule the repo chips have always drawn. + */ +export function repoEmphasis({ repo, subpath }: RepoPathRef): string { + if (subpath) return subpath; + const idx = repo.lastIndexOf('/'); + return idx >= 0 ? repo.slice(idx + 1) : repo; +} diff --git a/apps/staged/src/lib/stores/projectsData.svelte.ts b/apps/staged/src/lib/stores/projectsData.svelte.ts index 80957d76c..18a2b2f01 100644 --- a/apps/staged/src/lib/stores/projectsData.svelte.ts +++ b/apps/staged/src/lib/stores/projectsData.svelte.ts @@ -23,6 +23,14 @@ * View-lifecycle side effects (workspaceLifecycle.enqueueInitialSetup, * queued-session draining, run-action hydration) intentionally stay out of * the store — consuming views wire them by watching branchesByProject. + * + * Authoritative freshness comes from the store change feed (project-changed, + * branch-changed, repos-changed): every mutating backend store method + * publishes, so a write in any window — or in the backend itself — refetches + * here. The remaining imperative entry points (projectCreated, + * setBranchesByProject, refreshProject) exist for immediacy: they paint the + * local window's own mutation without waiting a coalescing window for the + * echo; the event-driven refetch then confirms. */ import { listenToEvent, type UnlistenFn } from '../transport'; @@ -30,10 +38,13 @@ import * as commands from '../commands'; import { repoBadgeStore } from './repoBadges.svelte'; import type { Branch, + BranchChangedEvent, PrStatusChangedEvent, Project, + ProjectChangedEvent, ProjectRepo, RepoHomeItem, + ReposChangedEvent, SessionStatusPayload, } from '../types'; @@ -122,6 +133,8 @@ class ProjectsDataStore { private loadGeneration = 0; private initialLoad: Promise | null = null; private revalidatePending = false; + private revalidateQueued = false; + private revalidateQueuedForce = false; private backgroundHydrationCancel: (() => void) | null = null; /** In-flight per-project hydrations, so the foreground fetch, the idle drip * and the grid's sweep share one request instead of racing three. */ @@ -134,6 +147,10 @@ class ProjectsDataStore { private unlisteners: UnlistenFn[] = []; private pendingPrStatusEvents: PrStatusChangedEvent[] = []; private prStatusFlushCancel: (() => void) | null = null; + private pendingBranchChanges: BranchChangedEvent[] = []; + private branchChangedFlushCancel: (() => void) | null = null; + private branchRefetchInFlight = new Set(); + private branchRefetchQueued = new Set(); // ── Reactive reads ── @@ -306,9 +323,9 @@ class ProjectsDataStore { const generation = this.loadGeneration; try { const [projectsResult, branchesResult, reposResult] = await Promise.all([ - commands.listProjects(), - commands.listBranchesForProject(projectId), - commands.listProjectRepos(projectId), + commands.listProjects({ force: true }), + commands.listBranchesForProject(projectId, { force: true }), + commands.listProjectRepos(projectId, { force: true }), ]); if (generation !== this.loadGeneration) return; this._projects = projectsResult.data; @@ -352,17 +369,31 @@ class ProjectsDataStore { this._branchesByProject = next; } - private async revalidate(): Promise { - if (this.revalidatePending) return; + private async revalidate({ force = false }: { force?: boolean } = {}): Promise { + if (this.revalidatePending) { + // A change arrived while a reload was in flight; that reload may have + // read the list before the write committed, so run once more after it. + // Preserve force across the queue: if any queued event says the backend + // changed, the follow-up fetch must not accept a cached answer. + this.revalidateQueued = true; + this.revalidateQueuedForce ||= force; + return; + } this.revalidatePending = true; try { - await this.loadProjectsAndHydrate(); + await this.loadProjectsAndHydrate({ force }); } finally { this.revalidatePending = false; + if (this.revalidateQueued) { + const queuedForce = this.revalidateQueuedForce; + this.revalidateQueued = false; + this.revalidateQueuedForce = false; + void this.revalidate({ force: queuedForce }); + } } } - private async loadProjectsAndHydrate(): Promise { + private async loadProjectsAndHydrate({ force = false }: { force?: boolean } = {}): Promise { const generation = ++this.loadGeneration; this.cancelBackgroundHydration(); // Those promises are already no-ops under the new generation; drop them so @@ -374,7 +405,7 @@ class ProjectsDataStore { this._error = null; await repoBadgeStore.loadAll(); try { - const { data, revalidating } = await commands.listProjects(); + const { data, revalidating } = await commands.listProjects({ force }); if (generation !== this.loadGeneration) return; this.applyProjectList(data, generation); this._loaded = true; @@ -428,6 +459,17 @@ class ProjectsDataStore { } this._hydratedProjects = prunedHydrated; + // A project that vanished from the fetched list finished deleting: drop + // its in-progress marker in the same apply, so the card goes straight + // from "Deleting…" to gone with no flash of a live project in between. + if (this._deletingProjectNames.size > 0) { + const prunedDeleting = new Map(); + for (const [projectId, name] of this._deletingProjectNames) { + if (projectIds.has(projectId)) prunedDeleting.set(projectId, name); + } + this._deletingProjectNames = prunedDeleting; + } + this.scheduleBackgroundHydration( projectList.map((p) => p.id), generation @@ -613,52 +655,39 @@ class ProjectsDataStore { // ── Project-delete lifecycle ── // - // Replaces the staged:project-delete-start/end window-event relay between - // ProjectsList and ProjectHome: the delete flow calls these directly and - // every consumer sees the same deletingProjectNames. + // On the success path the data side is event-driven: the backend delete + // publishes project-changed, whose list refetch removes the project and + // its "Deleting…" marker in one apply (see applyProjectList). Until that + // lands, the marker keeps the card in its deleting state — even if a fetch + // started before the delete resolves late and still contains the project — + // so nothing ever flashes back to life. + // + // The marker is not purely cosmetic, though: flushBranchChanges suppresses + // branch refetches for a deleting project, since the cascade emits one + // branch-changed per branch and refetching a doomed list N times would be + // pure churn. That debt has to be paid back when the delete fails — see + // projectDeleteFailed. projectDeleteStarted(projectId: string, name: string): void { this._deletingProjectNames = new Map(this._deletingProjectNames).set(projectId, name); } - /** Mark a delete finished. `removed` prunes the project from the store - * (backend deletion succeeded); omit it when the delete failed. */ - projectDeleteFinished(projectId: string, options: { removed?: boolean } = {}): void { + /** Clear the in-progress marker for a delete that failed — the project is + * still alive, so no project-changed refetch will prune it. The cascade may + * have deleted branch rows before failing, and flushBranchChanges dropped + * those branch-changed events on the floor, so refetch the list here: it is + * the only signal that will ever repair it. Clearing the marker first also + * lets any trailing branch-changed from the same cascade take the normal + * path instead of being dropped a second time. */ + projectDeleteFailed(projectId: string): void { const next = new Map(this._deletingProjectNames); next.delete(projectId); this._deletingProjectNames = next; - if (options.removed) { - this.removeProject(projectId); - } - } - - private removeProject(projectId: string): void { - // Bump the generation so any list or branch apply already in flight — an - // SWR `revalidating` promise, a concurrent ensureLoaded() revalidation, - // refreshProject's un-deduped list replacement — is discarded instead of - // resurrecting the project it fetched before the backend delete. - const generation = ++this.loadGeneration; - // In-flight hydrations are no-ops under the new generation; drop them so - // callers after the bump start fresh fetches. - this.hydrationInFlight.clear(); - this._projects = this._projects.filter((p) => p.id !== projectId); - const branches = new Map(this._branchesByProject); - branches.delete(projectId); - this._branchesByProject = branches; - const repos = new Map(this._reposByProject); - repos.delete(projectId); - this._reposByProject = repos; - const hydrated = new Map(this._hydratedProjects); - hydrated.delete(projectId); - this._hydratedProjects = hydrated; - // The bump halted the running idle drip too. Restart it for whatever is - // still un-hydrated — including a first hydration the bump just discarded - // — but not for hydrated projects: a delete doesn't stale their data, so - // this isn't the refetch-everything drip a fresh list apply schedules. - this.scheduleBackgroundHydration( - this._projects.filter((p) => !this._hydratedProjects.has(p.id)).map((p) => p.id), - generation - ); + // Same guard as flushBranchChanges: an un-hydrated project has nothing + // painted to repair. In practice this always fires — the delete flow awaits + // ensureProjectHydrated before starting — so the guard is consistency, not + // a live filter here. + this.refetchBranchesIfHydrated(projectId); } // ── Event listeners ── @@ -685,7 +714,7 @@ class ProjectsDataStore { // sprout/draft-PR icon flips as soon as the first commit lands. this.unlisteners.push( listenToEvent('session-status-changed', (payload) => { - void this.handleCommitSessionCompleted(payload); + this.handleCommitSessionCompleted(payload); }) ); @@ -698,21 +727,42 @@ class ProjectsDataStore { }) ); + // Store change feed: a project write in any window (or the backend) + // reloads the list. Create/rename land the new row; a delete's refetch + // prunes the project and its "Deleting…" marker together, and the + // generation bump discards any stale apply still in flight. + this.unlisteners.push( + listenToEvent('project-changed', () => { + void this.revalidate({ force: true }); + }) + ); + + // Store change feed: refetch the branch lists a branch write touched. + // Buffered per frame like pr-status-changed — a bulk operation arrives + // as one event per branch, and each refetch replaces the whole list. + this.unlisteners.push( + listenToEvent('branch-changed', (payload) => { + this.pendingBranchChanges.push(payload); + this.branchChangedFlushCancel ??= scheduleFrame(() => this.flushBranchChanges()); + }) + ); + + // Store change feed: badges and the home repo list (pins, recents, + // affinities) are all repo writes. + this.unlisteners.push( + listenToEvent('repos-changed', () => { + void repoBadgeStore.loadAll(); + if (this._homeRepos !== null || this.homeReposInFlight) { + void this.startHomeReposFetch(); + } + }) + ); + const onCacheStale = () => { void this.refresh(); }; window.addEventListener('cache-stale', onCacheStale); this.unlisteners.push(() => window.removeEventListener('cache-stale', onCacheStale)); - - const onPinnedReposChanged = () => { - if (this._homeRepos !== null || this.homeReposInFlight) { - void this.startHomeReposFetch(); - } - }; - window.addEventListener('staged:pinned-repos-changed', onPinnedReposChanged); - this.unlisteners.push(() => - window.removeEventListener('staged:pinned-repos-changed', onPinnedReposChanged) - ); } /** Tear down all listeners (tests, symmetry with startListeners). */ @@ -724,6 +774,9 @@ class ProjectsDataStore { this.prStatusFlushCancel?.(); this.prStatusFlushCancel = null; this.pendingPrStatusEvents = []; + this.branchChangedFlushCancel?.(); + this.branchChangedFlushCancel = null; + this.pendingBranchChanges = []; this.cancelBackgroundHydration(); this.listening = false; } @@ -761,14 +814,97 @@ class ProjectsDataStore { this._branchesByProject = next; } - private async handleCommitSessionCompleted(payload: SessionStatusPayload): Promise { + private handleCommitSessionCompleted(payload: SessionStatusPayload): void { if (payload.status !== 'completed') return; if (payload.sessionType !== 'commit') return; const projectId = payload.projectId; - if (!projectId || !this._branchesByProject.has(projectId)) return; + if (!projectId) return; + this.refetchBranchesIfHydrated(projectId); + } + + /** + * Map a burst of branch-changed events to the set of projects whose branch + * lists need refetching: the project the backend resolved, plus any known + * project whose list still holds the branch. The feed names every project a + * write touches — a move publishes once per side — so that scan is the + * fallback for what the backend couldn't see: an unresolved projectId, or a + * list this window holds that no longer matches the row. A null branchId is + * the feed's lag recovery — it can't name what it dropped, so every hydrated + * list is suspect. Collection stays broad; the single hydration filter is in + * refetchBranchesIfHydrated. + */ + private flushBranchChanges(): void { + this.branchChangedFlushCancel = null; + const events = this.pendingBranchChanges; + this.pendingBranchChanges = []; + if (events.length === 0) return; + + const projectIds = new Set(); + for (const { branchId, projectId } of events) { + if (branchId === null) { + for (const knownProjectId of this._branchesByProject.keys()) { + projectIds.add(knownProjectId); + } + continue; + } + if (projectId) projectIds.add(projectId); + for (const [knownProjectId, branches] of this._branchesByProject) { + if (branches.some((b) => b.id === branchId)) projectIds.add(knownProjectId); + } + } + for (const projectId of projectIds) { + // A deleting project's teardown emits one event per branch — refetching a + // list that's mid-cascade-delete would just churn. If that delete fails, + // projectDeleteFailed refetches once to repair what was skipped. + if (this.isProjectDeleting(projectId)) continue; + this.refetchBranchesIfHydrated(projectId); + } + } + + /** + * Refetch a project's branch list only when the store holds — or is about to + * hold — real fetched data to repair. + * + * Hydrated: refetch now. First hydration in flight: refetch once it settles, + * so this event's fresh read applies last — the in-flight fetch may have read + * pre-mutation state, and skipping outright would leave that stale read + * painted until the project's next event. Otherwise skip: the entry + * applyProjectList seeds for every listed project is `[]`, which no view ever + * painted (they gate on isProjectHydrated), and the eventual first hydration + * reads post-event state anyway. Fetching there would also half-hydrate the + * project — branches without repos, still unmarked — so the idle drip would + * refetch it regardless. + * + * Unknown projects are subsumed: hydrated ⊆ known, since markProjectHydrated + * is only reached for listed or just-created projects and applyProjectList + * prunes _hydratedProjects against the same fetched list. + */ + private refetchBranchesIfHydrated(projectId: string): void { + if (this._hydratedProjects.has(projectId)) { + void this.refetchProjectBranches(projectId); + return; + } + const inFlight = this.hydrationInFlight.get(projectId); + if (inFlight) { + void inFlight.then(() => this.refetchProjectBranches(projectId)); + } + } + + /** Refetch one project's branch list and apply it under the current load + * generation. */ + private async refetchProjectBranches(projectId: string): Promise { + if (this.branchRefetchInFlight.has(projectId)) { + // The pending read may predate this change. Run one more fetch after it + // settles rather than racing two responses that can apply out of order. + this.branchRefetchQueued.add(projectId); + return; + } + this.branchRefetchInFlight.add(projectId); const generation = this.loadGeneration; try { - const { data: branches, revalidating } = await commands.listBranchesForProject(projectId); + const { data: branches, revalidating } = await commands.listBranchesForProject(projectId, { + force: true, + }); this.applyProjectBranches(projectId, branches, generation); if (revalidating) { revalidating @@ -777,16 +913,18 @@ class ProjectsDataStore { }) .catch((e) => { console.error( - `[projectsData] Failed to revalidate branches for project ${projectId} after commit:`, + `[projectsData] Failed to revalidate branches for project ${projectId}:`, e ); }); } } catch (e) { - console.error( - `[projectsData] Failed to refresh branches for project ${projectId} after commit:`, - e - ); + console.error(`[projectsData] Failed to refetch branches for project ${projectId}:`, e); + } finally { + this.branchRefetchInFlight.delete(projectId); + if (this.branchRefetchQueued.delete(projectId)) { + void this.refetchProjectBranches(projectId); + } } } } diff --git a/apps/staged/src/lib/stores/projectsData.test.ts b/apps/staged/src/lib/stores/projectsData.test.ts index 93627f3d2..c43d598bb 100644 --- a/apps/staged/src/lib/stores/projectsData.test.ts +++ b/apps/staged/src/lib/stores/projectsData.test.ts @@ -113,6 +113,7 @@ let listProjectRepos: ReturnType; let listReposForHome: ReturnType; let invalidateProjectBranchTimelines: ReturnType; let ensureForRepos: ReturnType; +let badgeLoadAll: ReturnType; let eventListeners: Map; let windowTarget: EventTarget; @@ -126,6 +127,17 @@ function tick(): Promise { return new Promise((resolve) => setTimeout(resolve, 0)); } +/** + * Freeze the idle drip, so a test can hold a project in the known-but- + * un-hydrated state the hydration guard turns on. scheduleDeferredTask prefers + * requestIdleCallback over its setTimeout fallback, so a stub that never + * invokes its callback means the drip is scheduled and never runs. + */ +function freezeBackgroundHydration(): void { + vi.stubGlobal('requestIdleCallback', () => 1); + vi.stubGlobal('cancelIdleCallback', () => {}); +} + async function importStore() { const { projectsDataStore } = await import('./projectsData.svelte'); return projectsDataStore; @@ -146,6 +158,7 @@ beforeEach(() => { listReposForHome = vi.fn().mockResolvedValue([]); invalidateProjectBranchTimelines = vi.fn(); ensureForRepos = vi.fn().mockResolvedValue(undefined); + badgeLoadAll = vi.fn().mockResolvedValue(undefined); vi.doMock('../commands', () => ({ listProjects, @@ -167,7 +180,7 @@ beforeEach(() => { })); vi.doMock('./repoBadges.svelte', () => ({ repoBadgeStore: { - loadAll: vi.fn().mockResolvedValue(undefined), + loadAll: badgeLoadAll, ensureForRepos, }, })); @@ -554,6 +567,9 @@ describe('refreshProject', () => { ); await store.refreshProject('p1'); + expect(listProjects).toHaveBeenLastCalledWith({ force: true }); + expect(listBranchesForProject).toHaveBeenLastCalledWith('p1', { force: true }); + expect(listProjectRepos).toHaveBeenLastCalledWith('p1', { force: true }); expect(store.projects[0].name).toBe('Renamed'); expect(store.branchesByProject.get('p1')).toHaveLength(2); expect(invalidateProjectBranchTimelines).toHaveBeenCalledWith(['b1', 'b2']); @@ -683,6 +699,9 @@ describe('event listeners', () => { expect(eventListeners.get('pr-status-changed')).toHaveLength(1); expect(eventListeners.get('session-status-changed')).toHaveLength(1); expect(eventListeners.get('project-setup-progress')).toHaveLength(1); + expect(eventListeners.get('project-changed')).toHaveLength(1); + expect(eventListeners.get('branch-changed')).toHaveLength(1); + expect(eventListeners.get('repos-changed')).toHaveLength(1); }); it('coalesces a pr-status-changed burst into one flush, last event winning', async () => { @@ -724,12 +743,15 @@ describe('event listeners', () => { expect(store.branchesByProject.get('p1')![0].prState).toBe('OPEN'); }); expect(listBranchesForProject).toHaveBeenCalledTimes(1); + expect(listBranchesForProject).toHaveBeenCalledWith('p1', { force: true }); }); - it('ignores non-commit sessions and unknown projects', async () => { + it('ignores non-commit sessions and un-hydrated projects', async () => { + freezeBackgroundHydration(); + listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); const store = await importStore(); await store.ensureLoaded(); - await store.ensureProjectsHydrated(); + await store.ensureProjectHydrated('p1'); store.startListeners(); listBranchesForProject.mockClear(); @@ -745,6 +767,14 @@ describe('event listeners', () => { sessionType: 'commit', projectId: 'unknown', } satisfies SessionStatusPayload); + // p2 is listed but was never hydrated, so no sprout icon is painted for it + // — there is nothing for the refetch to flip. + emit('session-status-changed', { + sessionId: 's3', + status: 'completed', + sessionType: 'commit', + projectId: 'p2', + } satisfies SessionStatusPayload); await tick(); expect(listBranchesForProject).not.toHaveBeenCalled(); @@ -765,6 +795,9 @@ describe('event listeners', () => { await vi.waitFor(() => { expect(store.branchesByProject.get('p1')).toHaveLength(2); }); + expect(listProjects).toHaveBeenLastCalledWith({ force: true }); + expect(listBranchesForProject).toHaveBeenLastCalledWith('p1', { force: true }); + expect(listProjectRepos).toHaveBeenLastCalledWith('p1', { force: true }); expect(store.projects[0].name).toBe('Renamed'); expect(invalidateProjectBranchTimelines).toHaveBeenCalledWith(['b1', 'b2']); }); @@ -782,25 +815,246 @@ describe('event listeners', () => { }); }); - it('refetches home repos when pinned repos change', async () => { + it('reloads the project list on project-changed', async () => { + const store = await importStore(); + await store.ensureLoaded(); + store.startListeners(); + + listProjects.mockResolvedValue(swr([project({ name: 'Renamed' })])); + emit('project-changed', { projectId: 'p1' }); + + await vi.waitFor(() => { + expect(store.projects[0].name).toBe('Renamed'); + }); + expect(listProjects).toHaveBeenLastCalledWith({ force: true }); + }); + + it('registers a project created in another window on project-changed', async () => { + const store = await importStore(); + await store.ensureLoaded(); + store.startListeners(); + + listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); + emit('project-changed', { projectId: 'p2' }); + + await vi.waitFor(() => { + expect(store.projects.map((p) => p.id)).toEqual(['p1', 'p2']); + }); + }); + + it('preserves force for a project-changed refetch queued behind an in-flight reload', async () => { + const store = await importStore(); + await store.ensureLoaded(); + store.startListeners(); + + let resolveFirst!: (value: SwrResult) => void; + listProjects.mockReturnValueOnce( + new Promise>((resolve) => { + resolveFirst = resolve; + }) + ); + + // Remount-time revalidation starts unforced. + void store.ensureLoaded(); + await tick(); + expect(listProjects).toHaveBeenLastCalledWith({ force: false }); + + // An event arriving while it is pending queues a forced follow-up. + emit('project-changed', { projectId: 'p2' }); + await tick(); + expect(listProjects).toHaveBeenCalledTimes(2); + + listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); + resolveFirst(swr([project()])); + + await vi.waitFor(() => { + expect(listProjects).toHaveBeenCalledTimes(3); + }); + expect(listProjects).toHaveBeenLastCalledWith({ force: true }); + await vi.waitFor(() => { + expect(store.projects.map((p) => p.id)).toEqual(['p1', 'p2']); + }); + }); + + it('refetches the resolved project’s branches on branch-changed, coalescing a burst', async () => { + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + store.startListeners(); + listBranchesForProject.mockClear(); + listBranchesForProject.mockResolvedValue(swr([branch({ branchName: 'renamed' })])); + + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + + await vi.waitFor(() => { + expect(store.branchesByProject.get('p1')![0].branchName).toBe('renamed'); + }); + expect(listBranchesForProject).toHaveBeenCalledTimes(1); + expect(listBranchesForProject).toHaveBeenCalledWith('p1', { force: true }); + }); + + it('queues a branch refetch when another change arrives during an in-flight read', async () => { + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + store.startListeners(); + listBranchesForProject.mockClear(); + + let resolveFirst!: (value: SwrResult) => void; + listBranchesForProject.mockReturnValueOnce( + new Promise>((resolve) => { + resolveFirst = resolve; + }) + ); + + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + await vi.waitFor(() => expect(listBranchesForProject).toHaveBeenCalledTimes(1)); + + // This change may have committed after the first read began. It must queue + // a follow-up rather than race a second response against the first. + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + await tick(); + expect(listBranchesForProject).toHaveBeenCalledTimes(1); + + listBranchesForProject.mockResolvedValue(swr([branch({ branchName: 'fresh' })])); + resolveFirst(swr([branch({ branchName: 'stale' })])); + + await vi.waitFor(() => expect(listBranchesForProject).toHaveBeenCalledTimes(2)); + await vi.waitFor(() => { + expect(store.branchesByProject.get('p1')![0].branchName).toBe('fresh'); + }); + }); + + it('falls back to projects holding the branch when branch-changed lacks a project', async () => { + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + store.startListeners(); + listBranchesForProject.mockClear(); + listBranchesForProject.mockResolvedValue(swr([])); + + emit('branch-changed', { branchId: 'b1', projectId: null }); + + await vi.waitFor(() => { + expect(store.branchesByProject.get('p1')).toHaveLength(0); + }); + expect(listBranchesForProject).toHaveBeenCalledTimes(1); + expect(listBranchesForProject).toHaveBeenCalledWith('p1', { force: true }); + }); + + it('refetches every hydrated project’s branches when branch-changed names none', async () => { + freezeBackgroundHydration(); + listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); + listBranchesForProject.mockImplementation((projectId: string) => + Promise.resolve(swr([branch({ id: `${projectId}-b1`, projectId })])) + ); + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectHydrated('p1'); + store.startListeners(); + listBranchesForProject.mockClear(); + + // The lag flush: the feed dropped changes it can no longer name, so it + // expands to every project the store paints — p2, never hydrated, is not + // one of them even though applyProjectList seeded it a branch entry. + emit('branch-changed', { branchId: null, projectId: null }); + await tick(); + + expect(listBranchesForProject.mock.calls).toEqual([['p1', { force: true }]]); + }); + + it('skips branch-changed refetches for a known but un-hydrated project', async () => { + freezeBackgroundHydration(); + listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectHydrated('p1'); + store.startListeners(); + listBranchesForProject.mockClear(); + + // The state _branchesByProject.has() cannot distinguish: p2 is in the + // fetched list, so it holds a seeded empty entry, but nobody ever fetched + // it and no view painted it. + expect(store.branchesByProject.has('p2')).toBe(true); + expect(store.isProjectHydrated('p2')).toBe(false); + + emit('branch-changed', { branchId: 'p2-b1', projectId: 'p2' }); + await tick(); + + expect(listBranchesForProject).not.toHaveBeenCalled(); + }); + + it('chains a branch-changed refetch onto a first hydration still in flight', async () => { + freezeBackgroundHydration(); + let resolveBranches!: (value: SwrResult) => void; + listBranchesForProject.mockReturnValueOnce( + new Promise>((resolve) => { + resolveBranches = resolve; + }) + ); + const store = await importStore(); + await store.ensureLoaded(); + store.startListeners(); + + // The first hydration is mid-flight, so its read may predate the mutation + // the event announces — skipping outright would paint that stale read and + // leave it until the project's next event. + const hydrating = store.hydrateProject('p1'); + expect(listBranchesForProject).toHaveBeenCalledTimes(1); + + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + await tick(); + expect(listBranchesForProject).toHaveBeenCalledTimes(1); + + listBranchesForProject.mockResolvedValue(swr([branch({ branchName: 'renamed' })])); + resolveBranches(swr([branch({ branchName: 'stale' })])); + await hydrating; + + // Exactly one follow-up, and it applies after the hydration it chained onto. + await vi.waitFor(() => { + expect(store.branchesByProject.get('p1')![0].branchName).toBe('renamed'); + }); + expect(listBranchesForProject).toHaveBeenCalledTimes(2); + expect(listBranchesForProject).toHaveBeenLastCalledWith('p1', { force: true }); + }); + + it('skips branch-changed refetches for unknown or deleting projects', async () => { + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + store.startListeners(); + listBranchesForProject.mockClear(); + + store.projectDeleteStarted('p1', 'Alpha'); + emit('branch-changed', { branchId: 'b1', projectId: 'p1' }); + emit('branch-changed', { branchId: 'b-elsewhere', projectId: 'unknown' }); + await tick(); + + expect(listBranchesForProject).not.toHaveBeenCalled(); + }); + + it('reloads badges and home repos on repos-changed', async () => { listReposForHome.mockResolvedValue([homeRepo()]); const store = await importStore(); store.startListeners(); await store.ensureHomeReposLoaded(); + badgeLoadAll.mockClear(); listReposForHome.mockResolvedValue([homeRepo({ pinned: true })]); - windowTarget.dispatchEvent(new Event('staged:pinned-repos-changed')); + emit('repos-changed', { githubRepo: 'org/alpha' }); await vi.waitFor(() => { expect(store.homeRepos[0].pinned).toBe(true); }); + expect(badgeLoadAll).toHaveBeenCalledTimes(1); }); - it('does not fetch home repos on pin changes before anyone loaded them', async () => { + it('does not fetch home repos on repos-changed before anyone loaded them', async () => { const store = await importStore(); store.startListeners(); - windowTarget.dispatchEvent(new Event('staged:pinned-repos-changed')); + emit('repos-changed', { githubRepo: 'org/alpha' }); await tick(); expect(listReposForHome).not.toHaveBeenCalled(); @@ -820,34 +1074,105 @@ describe('event listeners', () => { }); describe('project-delete lifecycle', () => { - it('tracks deleting projects and prunes state when removal completes', async () => { + it('tracks the deleting project until the post-delete refetch prunes it', async () => { const store = await importStore(); + store.startListeners(); await store.ensureLoaded(); store.projectDeleteStarted('p1', 'Alpha'); expect(store.isProjectDeleting('p1')).toBe(true); expect(store.deletingProjectNames.get('p1')).toBe('Alpha'); - store.projectDeleteFinished('p1', { removed: true }); + // The backend delete publishes project-changed; the refetch removes the + // project and its "Deleting…" marker in one apply. + listProjects.mockResolvedValue(swr([])); + emit('project-changed', { projectId: 'p1' }); + + await vi.waitFor(() => { + expect(store.projects).toHaveLength(0); + }); expect(store.isProjectDeleting('p1')).toBe(false); - expect(store.projects).toHaveLength(0); expect(store.branchesByProject.has('p1')).toBe(false); expect(store.reposByProject.has('p1')).toBe(false); }); - it('keeps the project when a delete fails', async () => { + it('keeps the project and repairs its branch list when a delete fails', async () => { + listBranchesForProject.mockResolvedValue(swr([branch(), branch({ id: 'b2' })])); const store = await importStore(); + store.startListeners(); await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + listBranchesForProject.mockClear(); store.projectDeleteStarted('p1', 'Alpha'); - store.projectDeleteFinished('p1'); + + // The cascade's per-branch events are dropped while the delete is in + // flight — refetching a doomed list N times would be pure churn. + emit('branch-changed', { branchId: 'b2', projectId: 'p1' }); + await tick(); + expect(listBranchesForProject).not.toHaveBeenCalled(); + + // The delete failed after the cascade already deleted b2's row, so the + // dropped events are the ones that would have pruned it: clearing the + // marker has to refetch, or the card lists a branch that no longer exists. + listBranchesForProject.mockResolvedValue(swr([branch()])); + store.projectDeleteFailed('p1'); expect(store.isProjectDeleting('p1')).toBe(false); expect(store.projects).toHaveLength(1); + expect(listBranchesForProject).toHaveBeenCalledWith('p1', { force: true }); + await vi.waitFor(() => { + expect(store.branchesByProject.get('p1')!.map((b) => b.id)).toEqual(['b1']); + }); + }); + + it('does not fetch branches for an un-hydrated project when a delete fails', async () => { + const store = await importStore(); + await store.ensureLoaded(); + await store.ensureProjectsHydrated(); + listBranchesForProject.mockClear(); + + // No branch list was ever painted for this project, so there is nothing to + // repair — and fetching would insert a map entry the store never loaded. + store.projectDeleteFailed('p-unknown'); + await tick(); + + expect(listBranchesForProject).not.toHaveBeenCalled(); + expect(store.branchesByProject.has('p-unknown')).toBe(false); + }); + + it('does not fetch branches when a delete fails for a known but un-hydrated project', async () => { + freezeBackgroundHydration(); + const store = await importStore(); + await store.ensureLoaded(); + listBranchesForProject.mockClear(); + + // p1 is listed, so applyProjectList seeded it a branch entry — but nothing + // fetched or painted it, and a fetch here would half-hydrate it: branches + // without repos, unmarked, so the idle drip would refetch it anyway. expect(store.branchesByProject.has('p1')).toBe(true); + store.projectDeleteFailed('p1'); + await tick(); + + expect(listBranchesForProject).not.toHaveBeenCalled(); + }); + + it('keeps the deleting marker through an apply that still contains the project', async () => { + const store = await importStore(); + await store.ensureLoaded(); + + store.projectDeleteStarted('p1', 'Alpha'); + // A reload that read the list before the backend delete committed: the + // project stays, but so does its marker — the card must not flash back + // to a live project mid-delete. + await store.ensureLoaded(); // kicks a background revalidation + await tick(); + + expect(store.projects).toHaveLength(1); + expect(store.isProjectDeleting('p1')).toBe(true); }); - it('discards an SWR revalidation that resolves after the delete', async () => { + it('discards an SWR revalidation that resolves after the post-delete refetch', async () => { const beta = project({ id: 'p2', name: 'Beta' }); let resolveFresh!: (value: Project[]) => void; listProjects.mockResolvedValueOnce( @@ -859,46 +1184,31 @@ describe('project-delete lifecycle', () => { ) ); const store = await importStore(); + store.startListeners(); await store.ensureLoaded(); store.projectDeleteStarted('p2', 'Beta'); - store.projectDeleteFinished('p2', { removed: true }); + listProjects.mockResolvedValue(swr([project()])); + emit('project-changed', { projectId: 'p2' }); + await vi.waitFor(() => { + expect(store.projects.map((p) => p.id)).toEqual(['p1']); + }); - // Fetched before the backend delete — applying it would resurrect p2. + // Fetched before the backend delete — applying it would resurrect p2, + // but the post-delete reload's generation bump discards it. resolveFresh([project(), beta]); await tick(); expect(store.projects.map((p) => p.id)).toEqual(['p1']); expect(store.branchesByProject.has('p2')).toBe(false); - }); - - it('discards a concurrent ensureLoaded revalidation that resolves after the delete', async () => { - const beta = project({ id: 'p2', name: 'Beta' }); - listProjects.mockResolvedValue(swr([project(), beta])); - const store = await importStore(); - await store.ensureLoaded(); - - let resolveReload!: (value: SwrResult) => void; - listProjects.mockReturnValueOnce( - new Promise>((resolve) => { - resolveReload = resolve; - }) - ); - await store.ensureLoaded(); // kicks the background revalidation - - store.projectDeleteStarted('p2', 'Beta'); - store.projectDeleteFinished('p2', { removed: true }); - - resolveReload(swr([project(), beta])); - await tick(); - - expect(store.projects.map((p) => p.id)).toEqual(['p1']); + expect(store.isProjectDeleting('p2')).toBe(false); }); it("discards refreshProject's list replacement racing the delete", async () => { const beta = project({ id: 'p2', name: 'Beta' }); listProjects.mockResolvedValue(swr([project(), beta])); const store = await importStore(); + store.startListeners(); await store.ensureLoaded(); let resolveList!: (value: SwrResult) => void; @@ -910,42 +1220,17 @@ describe('project-delete lifecycle', () => { const refreshing = store.refreshProject('p1'); store.projectDeleteStarted('p2', 'Beta'); - store.projectDeleteFinished('p2', { removed: true }); + listProjects.mockResolvedValue(swr([project()])); + emit('project-changed', { projectId: 'p2' }); + await vi.waitFor(() => { + expect(store.projects.map((p) => p.id)).toEqual(['p1']); + }); resolveList(swr([project(), beta])); await refreshing; expect(store.projects.map((p) => p.id)).toEqual(['p1']); }); - - it('restarts the idle drip so un-hydrated projects still fill in after a delete', async () => { - listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); - const store = await importStore(); - await store.ensureLoaded(); - expect(store.isProjectHydrated('p1')).toBe(false); - - store.projectDeleteStarted('p2', 'Beta'); - store.projectDeleteFinished('p2', { removed: true }); - - await vi.waitFor(() => { - expect(store.isProjectHydrated('p1')).toBe(true); - }); - expect(listBranchesForProject).not.toHaveBeenCalledWith('p2'); - }); - - it('does not refetch already-hydrated projects after a delete', async () => { - listProjects.mockResolvedValue(swr([project(), project({ id: 'p2', name: 'Beta' })])); - const store = await importStore(); - await store.ensureLoaded(); - await store.ensureProjectsHydrated(); - listBranchesForProject.mockClear(); - - store.projectDeleteStarted('p2', 'Beta'); - store.projectDeleteFinished('p2', { removed: true }); - await tick(); - - expect(listBranchesForProject).not.toHaveBeenCalled(); - }); }); describe('repoCountsByProject', () => { diff --git a/apps/staged/src/lib/transport.test.ts b/apps/staged/src/lib/transport.test.ts index fa1cf31f6..80d27b220 100644 --- a/apps/staged/src/lib/transport.test.ts +++ b/apps/staged/src/lib/transport.test.ts @@ -1,5 +1,5 @@ // @vitest-environment jsdom -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest'; class MockWebSocket { static CONNECTING = 0; @@ -49,6 +49,7 @@ let sockets: MockWebSocket[]; describe('web transport', () => { let hydrateActiveSessions: ReturnType; + let revalidateAll: ReturnType; beforeEach(() => { vi.resetModules(); @@ -58,11 +59,14 @@ describe('web transport', () => { // can't compile, so it is mocked for every socket-opening test. hydrateActiveSessions = vi.fn().mockResolvedValue(undefined); vi.doMock('./listeners/sessionStatusListener', () => ({ hydrateActiveSessions })); + revalidateAll = vi.fn().mockResolvedValue(undefined); + vi.doMock('./listeners/pageLifecycleListener', () => ({ revalidateAll })); }); afterEach(() => { vi.doUnmock('./services/prPollingService'); vi.doUnmock('./listeners/sessionStatusListener'); + vi.doUnmock('./listeners/pageLifecycleListener'); vi.unstubAllGlobals(); vi.useRealTimers(); }); @@ -146,7 +150,127 @@ describe('web transport', () => { sockets[1].open(); await vi.waitFor(() => expect(replayPrPollInterestHints).toHaveBeenCalledTimes(2)); await vi.waitFor(() => expect(hydrateActiveSessions).toHaveBeenCalledTimes(2)); + await vi.waitFor(() => expect(revalidateAll).toHaveBeenCalledTimes(1)); unlisten(); }); + + it('revalidates every cached surface on reconnect but not on the first connect', async () => { + vi.useFakeTimers(); + vi.stubGlobal('WebSocket', MockWebSocket); + const { listenToEvent } = await import('./transport'); + const unlisten = listenToEvent('pr-refresh-state', vi.fn()); + await vi.waitFor(() => expect(sockets).toHaveLength(1)); + + // First connect: the page's own loads are already current. + sockets[0].open(); + await vi.waitFor(() => expect(hydrateActiveSessions).toHaveBeenCalledTimes(1)); + expect(revalidateAll).not.toHaveBeenCalled(); + + // Reconnect: every event emitted during the gap is unrecoverable. + sockets[0].close(); + await vi.advanceTimersByTimeAsync(2000); + await vi.waitFor(() => expect(sockets).toHaveLength(2)); + + sockets[1].open(); + await vi.waitFor(() => expect(revalidateAll).toHaveBeenCalledTimes(1)); + + unlisten(); + }); + + it('recovers when the server reports dropped events without reconnecting', async () => { + vi.useFakeTimers(); + vi.stubGlobal('WebSocket', MockWebSocket); + const { listenToEvent } = await import('./transport'); + const callback = vi.fn(); + const unlisten = listenToEvent('project-changed', callback); + await vi.waitFor(() => expect(sockets).toHaveLength(1)); + + sockets[0].open(); + await vi.waitFor(() => expect(hydrateActiveSessions).toHaveBeenCalledTimes(1)); + + sockets[0].emit({ event: 'transport:event-gap', payload: null }); + + await vi.waitFor(() => expect(hydrateActiveSessions).toHaveBeenCalledTimes(2)); + await vi.waitFor(() => expect(revalidateAll).toHaveBeenCalledTimes(1)); + expect(callback).not.toHaveBeenCalled(); + expect(sockets).toHaveLength(1); + + unlisten(); + }); +}); + +describe('tauri window label', () => { + let consoleError: MockInstance; + + beforeEach(() => { + // Resets the module-level once-flag along with the module. + vi.resetModules(); + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.doUnmock('@tauri-apps/api/window'); + vi.unstubAllGlobals(); + consoleError.mockRestore(); + }); + + it('reads the label from the injected internals', async () => { + vi.stubGlobal('__TAURI__', {}); + vi.stubGlobal('__TAURI_INTERNALS__', { metadata: { currentWindow: { label: 'win-2' } } }); + + const { getWindowLabel } = await import('./transport'); + + expect(getWindowLabel()).toBe('win-2'); + expect(consoleError).not.toHaveBeenCalled(); + }); + + it('reads the internals path the installed @tauri-apps/api reads', async () => { + // The reshape tripwire. getWindowLabel() goes through the private + // internals rather than getCurrentWindow() because the label has to be + // available synchronously at module-init time, so an upgrade that moves + // the label would otherwise surface only at runtime, in a user's second + // window. getCurrentWindow() reads the same globals off the same shape, + // so pointing the real (unmocked) package at the stub below fails here — + // in CI, on the bump — instead. `skip: true` keeps it IPC-free. + vi.stubGlobal('__TAURI__', {}); + vi.stubGlobal('__TAURI_INTERNALS__', { metadata: { currentWindow: { label: 'win-2' } } }); + + const { getWindowLabel } = await import('./transport'); + const { getCurrentWindow } = await import('@tauri-apps/api/window'); + + expect(getCurrentWindow().label).toBe('win-2'); + expect(getWindowLabel()).toBe(getCurrentWindow().label); + expect(consoleError).not.toHaveBeenCalled(); + }); + + it('reports reshaped internals once, naming the label the official API sees', async () => { + vi.stubGlobal('__TAURI__', {}); + // The shape a Tauri upgrade might move the label to. + vi.stubGlobal('__TAURI_INTERNALS__', { metadata: { window: { label: 'win-2' } } }); + vi.doMock('@tauri-apps/api/window', () => ({ + getCurrentWindow: () => ({ label: 'win-2' }), + })); + + const { getWindowLabel } = await import('./transport'); + + expect(getWindowLabel()).toBeNull(); + expect(consoleError).toHaveBeenCalledTimes(1); + expect(consoleError.mock.calls[0][0]).toContain('Tauri window label unavailable'); + + await vi.waitFor(() => expect(consoleError).toHaveBeenCalledTimes(2)); + expect(consoleError.mock.calls[1][0]).toContain('the current window label is "win-2"'); + + // Every navigation persist calls through here, so a second failed lookup + // must stay quiet. + expect(getWindowLabel()).toBeNull(); + expect(consoleError).toHaveBeenCalledTimes(2); + }); + + it('stays silent in web mode, where a null label is the expected answer', async () => { + const { getWindowLabel } = await import('./transport'); + + expect(getWindowLabel()).toBeNull(); + expect(consoleError).not.toHaveBeenCalled(); + }); }); diff --git a/apps/staged/src/lib/transport.ts b/apps/staged/src/lib/transport.ts index 246a5061f..54d6c6944 100644 --- a/apps/staged/src/lib/transport.ts +++ b/apps/staged/src/lib/transport.ts @@ -15,6 +15,70 @@ export const isTauri: boolean = typeof window !== 'undefined' && '__TAURI__' in window; +let reportedMissingWindowLabel = false; + +/** + * The current Tauri window's label (`main`, `win-2`, …), or `null` in web + * mode. Read synchronously from the internals Tauri injects before any script + * runs (the same lookup `@tauri-apps/api/window` does), so it is safe to call + * at module-initialization time. + * + * Fails loudly. `null` in web mode is the expected answer and is returned + * silently, but `null` in Tauri mode means the internals were reshaped (most + * likely by an upgrade) and every window is about to collapse to the shared + * `main` identity — a regression whose only symptom would otherwise be + * mysteriously wrong PR-poll cadence. That case is reported via + * `console.error` once per page, followed by the label the official API sees. + */ +export function getWindowLabel(): string | null { + if (!isTauri) return null; + const internals = ( + window as unknown as { + __TAURI_INTERNALS__?: { metadata?: { currentWindow?: { label?: string } } }; + } + ).__TAURI_INTERNALS__; + const label = internals?.metadata?.currentWindow?.label ?? null; + if (label === null) reportMissingWindowLabel(); + return label; +} + +/** + * Report a failed label lookup exactly once per page. The once-flag matters + * because `persistLastProject()` calls `getWindowLabel()` on every navigation + * persist — unguarded, a broken upgrade would spam the console on every route + * change. + */ +function reportMissingWindowLabel(): void { + if (reportedMissingWindowLabel) return; + reportedMissingWindowLabel = true; + + console.error( + '[transport] Tauri window label unavailable — __TAURI_INTERNALS__.metadata.currentWindow.label is ' + + 'missing. All windows degrade to the shared "main" identity: PR-poll focus/selection hints will ' + + 'clobber each other across windows, all windows share the legacy last-viewed-project key, and ' + + "secondary windows won't consume their opener's project seed. Likely a Tauri upgrade reshaped the " + + 'internals; compare with getCurrentWindow() in @tauri-apps/api/window.' + ); + + // Diagnostic only. The npm package ships in lockstep with the Tauri release + // that injects the internals, so after a reshape it still reads the label + // correctly — turning "the read failed" into "the label is actually win-2, + // update getWindowLabel() to match". Deliberately not used to retro-correct: + // the PR-poll client id is already baked, and switching the navigation key + // mid-session would split reads and writes across two keys. + void import('@tauri-apps/api/window') + .then(({ getCurrentWindow }) => { + console.error( + `[transport] @tauri-apps/api reports the current window label is "${getCurrentWindow().label}" — ` + + 'update getWindowLabel() to read the new internals shape.' + ); + }) + .catch(() => { + // The official API is no more readable than the internals here; the + // error above is the whole diagnosis available. + }); +} + // --------------------------------------------------------------------------- // Command invocation // --------------------------------------------------------------------------- @@ -94,6 +158,37 @@ export function listenToEvent(event: string, callback: (payload: T) => void): }; } +/** + * Listen to a backend event addressed to *this window* (e.g. menu routing). + * `listenToEvent` registers an any-target listener, which Tauri matches + * against every emit — including ones targeted at other windows — so events + * that must reach exactly one window need this window-scoped variant paired + * with a backend `emit_to(label, ..)`. In web mode it falls back to the shared + * WebSocket stream, where the page is the only "window". + */ +export function listenToWindowEvent(event: string, callback: (payload: T) => void): UnlistenFn { + if (!isTauri) { + return webSocketListen(event, callback); + } + + let cancelled = false; + let unlisten: UnlistenFn | undefined; + + void (async () => { + const { getCurrentWindow } = await import('@tauri-apps/api/window'); + const u = await getCurrentWindow().listen(event, (e) => callback(e.payload)); + if (cancelled) u(); + else unlisten = u; + })().catch((e) => { + console.error(`[transport] Failed to register window listener for event "${event}":`, e); + }); + + return () => { + cancelled = true; + unlisten?.(); + }; +} + // --------------------------------------------------------------------------- // WebSocket singleton for web-mode events // --------------------------------------------------------------------------- @@ -104,12 +199,21 @@ interface WebSocketListener { } const WEB_SOCKET_HEARTBEAT_MS = 30_000; +const WEB_SOCKET_EVENT_GAP = 'transport:event-gap'; let ws: WebSocket | null = null; let wsListeners: WebSocketListener[] = []; let wsReconnectTimer: ReturnType | null = null; let wsHeartbeatTimer: ReturnType | null = null; let wsConnecting = false; +/** + * True once any socket has been created in this page's lifetime, so the next + * one can tell a first connect from a reconnect. Deliberately "a socket + * existed", not "a socket opened": a failed first attempt followed by a + * successful retry is also a gap, since HTTP loads complete happily while no + * feed is live. + */ +let wsHadSocket = false; async function getWsUrl(): Promise { const { getPrPollClientId } = await import('./services/prPollingService'); @@ -159,6 +263,34 @@ function rehydrateBusyState(): void { }); } +/** + * Revalidate every cached surface after a reconnect. The server keeps no + * per-client queue, so store change-feed events emitted while the socket was + * down are gone — and since those events are now the *only* thing that + * invalidates caches after a store-backed mutation (mutations themselves travel + * over HTTP, which works fine with the socket down), a lost echo strands the + * view indefinitely. Reuses the page-resume recovery, whose `cache-stale` + * consumers already cover every surface the feed feeds. + * + * No gap-duration threshold, unlike the resume path: there a brief tab switch + * leaves the socket alive, whereas here the socket was provably down, and even a + * two-second gap can swallow a mutation echo. Dynamically imported both to match + * the house style above and to avoid a static cycle (pageLifecycleListener + * imports `isTauri` from here). + */ +function revalidateAfterEventGap(): void { + void import('./listeners/pageLifecycleListener') + .then(({ revalidateAll }) => revalidateAll()) + .catch((e) => { + console.error('[transport] Failed to revalidate after event gap:', e); + }); +} + +function recoverAfterEventGap(): void { + rehydrateBusyState(); + revalidateAfterEventGap(); +} + async function ensureWebSocket(): Promise { if (ws && (ws.readyState === WebSocket.OPEN || ws.readyState === WebSocket.CONNECTING)) { return; @@ -172,6 +304,9 @@ async function ensureWebSocket(): Promise { return; } + const isReconnect = wsHadSocket; + wsHadSocket = true; + const socket = new WebSocket(url); ws = socket; @@ -179,7 +314,8 @@ async function ensureWebSocket(): Promise { wsConnecting = false; startHeartbeat(); replayCurrentPrPollInterestHints(); - rehydrateBusyState(); + if (isReconnect) recoverAfterEventGap(); + else rehydrateBusyState(); if (wsReconnectTimer) { clearTimeout(wsReconnectTimer); wsReconnectTimer = null; @@ -189,6 +325,10 @@ async function ensureWebSocket(): Promise { socket.onmessage = (messageEvent) => { try { const data = JSON.parse(messageEvent.data) as { event: string; payload: unknown }; + if (data.event === WEB_SOCKET_EVENT_GAP) { + recoverAfterEventGap(); + return; + } for (const listener of wsListeners) { if (listener.event === data.event) { listener.callback(data.payload); @@ -259,6 +399,7 @@ interface WindowHandle { close(): Promise; startDragging(): Promise; setBadgeCount(count: number | undefined): Promise; + setTitle(title: string): Promise; } const noopWindow: WindowHandle = { @@ -268,6 +409,11 @@ const noopWindow: WindowHandle = { }, startDragging: async () => {}, setBadgeCount: async () => {}, + // Not a no-op in web mode: the browser tab is the window list, so the title + // is a real affordance there. index.html ships the same default title. + setTitle: async (title: string) => { + if (typeof document !== 'undefined') document.title = title; + }, }; /** @@ -307,6 +453,10 @@ export function getWindowSync(): WindowHandle { const { getCurrentWindow } = await import('@tauri-apps/api/window'); return getCurrentWindow().setBadgeCount(count); }, + setTitle: async (title: string) => { + const { getCurrentWindow } = await import('@tauri-apps/api/window'); + return getCurrentWindow().setTitle(title); + }, }; } diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index 69cace856..2d023080d 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -118,6 +118,36 @@ export interface PrStatusChangedEvent { failedChecks: PrFailedCheck[]; } +// Store change feed events (src-tauri/src/store_events.rs). Every mutating +// store method publishes one, so these fire for a write made in any window or +// in the backend itself. A null id means the backend couldn't resolve it — +// treat as "refetch the whole surface". An event whose ids are *all* null is +// the feed's lag recovery: it dropped changes it can no longer describe, and +// every one of these fires at once. + +export interface ProjectChangedEvent { + projectId: string | null; +} + +export interface BranchChangedEvent { + branchId: string | null; + projectId: string | null; +} + +export interface NotesChangedEvent { + branchId: string | null; + projectId: string | null; +} + +export interface ReviewChangedEvent { + reviewId: string | null; + branchId: string | null; +} + +export interface ReposChangedEvent { + githubRepo: string | null; +} + export interface CommitTimelineItem { /** DB id — present for pending/failed commits so they can be deleted by id. */ id: string | null;