From 95e70ef52075480d8d62c56955b6205d2773d895 Mon Sep 17 00:00:00 2001 From: ddidderr Date: Sat, 16 May 2026 09:01:17 +0200 Subject: [PATCH] fix(ui): reconcile active operations from local scans Local operation spinners were driven by begin, finish, and failure event history. If one of those lifecycle events was missed, the Tauri bridge could keep a stale active operation and the React state would keep showing an in-progress spinner until restart. Peer local scan updates now carry an authoritative active-operation snapshot. The peer still suppresses active game roots from peer-facing library deltas, but it emits LocalGamesUpdated to the UI even when no library delta changed so the snapshot can clear stale state after rollback or completion. The Tauri bridge replaces its active-operation map from that snapshot, emits it with the games-list payload, and the React merge uses it to restore download, install, update, and uninstall spinners from current peer state rather than event history alone. This also enables the Tauri lib unit-test target so the reconciliation helper can stay covered by the workspace test recipe. Test Plan: - git diff --check - just fmt - just clippy - just test Follow-up-Plan: FOLLOW_UP_2.md --- Cargo.lock | 1 + FOLLOW_UP_2.md | 18 ++- crates/lanspread-peer/src/handlers.rs | 146 +++++++++++++++--- crates/lanspread-peer/src/lib.rs | 27 +++- .../src-tauri/Cargo.toml | 3 +- .../src-tauri/src/lib.rs | 137 +++++++++++++++- crates/lanspread-tauri-deno-ts/src/App.tsx | 90 ++++++++++- 7 files changed, 384 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9e6ddd9..a843d1a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2214,6 +2214,7 @@ dependencies = [ "lanspread-peer", "log", "mimalloc", + "serde", "tauri", "tauri-build", "tauri-plugin-dialog", diff --git a/FOLLOW_UP_2.md b/FOLLOW_UP_2.md index 38810e5..8ed0068 100644 --- a/FOLLOW_UP_2.md +++ b/FOLLOW_UP_2.md @@ -1,6 +1,8 @@ # Follow-up Plan #2 -State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are what's still open. +State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are +what's still open, plus notes for follow-up items completed after this file was +created. ## Context for next time @@ -8,14 +10,16 @@ Branch `p2p-codex-muenchhausen` has uncommitted work that completes the bulk of The OperationGuard ordering fix in `handlers.rs` is the structurally most important change: install/update/uninstall now drop the guard _before_ the finish event and `refresh_local_game`, so peers see settled state in the next announcement instead of waiting for a scan tick. Tests `*_refreshes_settled_state_after_guard_release` pin this. +## Completed after this file was created + +- Tauri-side `active_operations` reconciliation: `PeerEvent::LocalGamesUpdated` + now carries an authoritative active-operation snapshot, the Tauri bridge + replaces its UI operation map from that snapshot, and the React game-list + merge uses the snapshot to clear stale download/install/update/uninstall + spinners even when a begin/finish/fail event was missed. + ## Still open -### Correctness / runtime - -#### 1. Tauri-side `active_operations` has no reconciliation - -If a `PeerEvent::InstallGameFinished` / `…Failed` is dropped, the UI is stuck "installing" until app restart. Include an in-progress snapshot in `PeerEvent::LocalGamesUpdated` so the UI recomputes from authoritative state instead of accumulating from event history. (Was `#10` in original plan, deferred both rounds.) - ### Tests still missing #### 2. Install-recovery matrix (~10 rows) diff --git a/crates/lanspread-peer/src/handlers.rs b/crates/lanspread-peer/src/handlers.rs index 174a0a4..8486141 100644 --- a/crates/lanspread-peer/src/handlers.rs +++ b/crates/lanspread-peer/src/handlers.rs @@ -11,6 +11,8 @@ use lanspread_db::db::{GameDB, GameFileDescription}; use tokio::sync::{RwLock, mpsc::UnboundedSender}; use crate::{ + ActiveOperation, + ActiveOperationKind, InstallOperation, PeerEvent, context::{Ctx, OperationGuard, OperationKind}, @@ -553,20 +555,14 @@ pub async fn update_and_announce_games( revision, } = scan; - let active_ids = ctx - .active_operations - .read() - .await - .keys() - .cloned() - .collect::>(); - if !active_ids.is_empty() { + let active_operations = active_operation_snapshot(ctx).await; + if !active_operations.is_empty() { let previous = ctx.local_library.read().await.games.clone(); - for id in active_ids { - if let Some(summary) = previous.get(&id) { - summaries.insert(id, summary.clone()); + for id in active_operations.iter().map(|operation| &operation.id) { + if let Some(summary) = previous.get(id) { + summaries.insert(id.clone(), summary.clone()); } else { - summaries.remove(&id); + summaries.remove(id); } } game_db = GameDB::from(summaries.values().map(game_from_summary).collect()); @@ -577,10 +573,6 @@ pub async fn update_and_announce_games( library_guard.update_from_scan(summaries, revision) }; - let Some(delta) = delta else { - return; - }; - { let mut db_guard = ctx.local_game_db.write().await; *db_guard = Some(game_db.clone()); @@ -588,10 +580,17 @@ pub async fn update_and_announce_games( let all_games = game_db.all_games().into_iter().cloned().collect::>(); - if let Err(e) = tx_notify_ui.send(PeerEvent::LocalGamesUpdated(all_games.clone())) { + if let Err(e) = tx_notify_ui.send(PeerEvent::LocalGamesUpdated { + games: all_games.clone(), + active_operations, + }) { log::error!("Failed to send LocalGamesUpdated event: {e}"); } + let Some(delta) = delta else { + return; + }; + let peer_targets = { let db = ctx.peer_game_db.read().await; db.peer_identities() @@ -625,6 +624,28 @@ pub async fn update_and_announce_games( } } +async fn active_operation_snapshot(ctx: &Ctx) -> Vec { + let active_operations = ctx.active_operations.read().await; + let mut snapshot = active_operations + .iter() + .map(|(id, operation)| ActiveOperation { + id: id.clone(), + operation: active_operation_kind(*operation), + }) + .collect::>(); + snapshot.sort_by(|left, right| left.id.cmp(&right.id)); + snapshot +} + +fn active_operation_kind(operation: OperationKind) -> ActiveOperationKind { + match operation { + OperationKind::Downloading => ActiveOperationKind::Downloading, + OperationKind::Installing => ActiveOperationKind::Installing, + OperationKind::Updating => ActiveOperationKind::Updating, + OperationKind::Uninstalling => ActiveOperationKind::Uninstalling, + } +} + #[cfg(test)] mod tests { use std::{ @@ -710,9 +731,17 @@ mod tests { } fn assert_local_update(event: PeerEvent, installed: bool, downloaded: bool) { - let PeerEvent::LocalGamesUpdated(games) = event else { + let PeerEvent::LocalGamesUpdated { + games, + active_operations, + } = event + else { panic!("expected LocalGamesUpdated"); }; + assert!( + active_operations.is_empty(), + "settled local update should not report active operations" + ); let game = games .iter() .find(|game| game.id == "game") @@ -721,6 +750,87 @@ mod tests { assert_eq!(game.downloaded, downloaded); } + #[tokio::test] + async fn local_games_update_reports_authoritative_active_operations() { + let temp = TempDir::new("lanspread-handler-active-snapshot"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + + let ctx = test_ctx(temp.path().to_path_buf()); + ctx.active_operations + .write() + .await + .insert("game".to_string(), OperationKind::Installing); + let (tx, mut rx) = mpsc::unbounded_channel(); + let catalog = ctx.catalog.read().await.clone(); + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("scan should succeed"); + + update_and_announce_games(&ctx, &tx, scan).await; + + let PeerEvent::LocalGamesUpdated { + games, + active_operations, + } = recv_event(&mut rx).await + else { + panic!("expected LocalGamesUpdated"); + }; + assert!( + games.is_empty(), + "active game should keep its previous announced state" + ); + assert_eq!( + active_operations, + vec![ActiveOperation { + id: "game".to_string(), + operation: ActiveOperationKind::Installing, + }] + ); + } + + #[tokio::test] + async fn unchanged_scan_still_reports_active_operation_snapshot() { + let temp = TempDir::new("lanspread-handler-active-unchanged"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, mut rx) = mpsc::unbounded_channel(); + let catalog = ctx.catalog.read().await.clone(); + + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("first scan should succeed"); + update_and_announce_games(&ctx, &tx, scan).await; + assert_local_update(recv_event(&mut rx).await, false, true); + + ctx.active_operations + .write() + .await + .insert("game".to_string(), OperationKind::Updating); + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("second scan should succeed"); + update_and_announce_games(&ctx, &tx, scan).await; + + let PeerEvent::LocalGamesUpdated { + active_operations, .. + } = recv_event(&mut rx).await + else { + panic!("expected LocalGamesUpdated"); + }; + assert_eq!( + active_operations, + vec![ActiveOperation { + id: "game".to_string(), + operation: ActiveOperationKind::Updating, + }] + ); + } + #[tokio::test] async fn install_refreshes_settled_state_after_guard_release() { let temp = TempDir::new("lanspread-handler-install"); diff --git a/crates/lanspread-peer/src/lib.rs b/crates/lanspread-peer/src/lib.rs index b1f26e4..bb9be55 100644 --- a/crates/lanspread-peer/src/lib.rs +++ b/crates/lanspread-peer/src/lib.rs @@ -111,8 +111,11 @@ pub enum PeerEvent { PeerLost(SocketAddr), /// The total peer count has changed. PeerCountUpdated(usize), - /// Local games have been updated. - LocalGamesUpdated(Vec), + /// Local games have been scanned, with authoritative in-progress work. + LocalGamesUpdated { + games: Vec, + active_operations: Vec, + }, /// A required peer runtime component failed. RuntimeFailed { component: PeerRuntimeComponent, @@ -144,6 +147,26 @@ pub enum InstallOperation { Updating, } +/// In-progress operation snapshot attached to local library updates. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ActiveOperation { + pub id: String, + pub operation: ActiveOperationKind, +} + +/// Operation kinds visible to UI reconciliation. +#[derive(Clone, Copy, Debug, PartialEq, Eq, strum::IntoStaticStr)] +pub enum ActiveOperationKind { + /// Downloading or replacing archive files. + Downloading, + /// Extracting into a previously uninstalled game root. + Installing, + /// Replacing an existing `local/` install. + Updating, + /// Removing an existing `local/` install. + Uninstalling, +} + /// Commands sent to the peer system from the UI. #[derive(Clone, Debug)] pub enum PeerCommand { diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml b/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml index 6f7bc93..551ae01 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml +++ b/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml @@ -13,7 +13,7 @@ edition = "2024" # This seems to be only an issue on Windows, see https://github.com/rust-lang/cargo/issues/8519 name = "lanspread_tauri_deno_ts_lib" crate-type = ["staticlib", "cdylib", "rlib"] -test = false +test = true doctest = false [lints.clippy] @@ -36,6 +36,7 @@ base64 = { workspace = true } eyre = { workspace = true } log = { workspace = true } mimalloc = { workspace = true } +serde = { workspace = true } tauri = { workspace = true } tauri-plugin-log = { workspace = true } tauri-plugin-shell = { workspace = true } diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs index 59f3c00..e28cc42 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs +++ b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs @@ -17,6 +17,8 @@ use lanspread_db::db::{ GameFileDescription, }; use lanspread_peer::{ + ActiveOperation, + ActiveOperationKind, InstallOperation, PeerCommand, PeerEvent, @@ -49,7 +51,7 @@ struct LanSpreadState { struct PeerEventTx(UnboundedSender); -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize)] enum UiOperationKind { Downloading, Installing, @@ -57,6 +59,18 @@ enum UiOperationKind { Uninstalling, } +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize)] +struct UiActiveOperation { + id: String, + operation: UiOperationKind, +} + +#[derive(Clone, Debug, serde::Serialize)] +struct GamesListPayload { + games: Vec, + active_operations: Vec, +} + struct SidecarUnpacker { app_handle: AppHandle, } @@ -527,13 +541,59 @@ async fn refresh_games_list(app_handle: &AppHandle) { drop(game_db); - if let Err(e) = app_handle.emit("games-list-updated", Some(games_to_emit)) { + let active_operations = { + let active_operations = state.active_operations.read().await; + ui_active_operations_from_map(&active_operations) + }; + + let payload = GamesListPayload { + games: games_to_emit, + active_operations, + }; + + if let Err(e) = app_handle.emit("games-list-updated", Some(payload)) { log::error!("Failed to emit games-list-updated event: {e}"); } else { log::info!("Emitted games-list-updated event"); } } +fn ui_active_operations_from_map( + active_operations: &HashMap, +) -> Vec { + let mut snapshot = active_operations + .iter() + .map(|(id, operation)| UiActiveOperation { + id: id.clone(), + operation: *operation, + }) + .collect::>(); + snapshot.sort_by(|left, right| left.id.cmp(&right.id)); + snapshot +} + +fn reconcile_active_operations( + active_operations: &mut HashMap, + snapshot: &[ActiveOperation], +) { + active_operations.clear(); + active_operations.extend(snapshot.iter().map(|operation| { + ( + operation.id.clone(), + ui_operation_from_peer(operation.operation), + ) + })); +} + +fn ui_operation_from_peer(operation: ActiveOperationKind) -> UiOperationKind { + match operation { + ActiveOperationKind::Downloading => UiOperationKind::Downloading, + ActiveOperationKind::Installing => UiOperationKind::Installing, + ActiveOperationKind::Updating => UiOperationKind::Updating, + ActiveOperationKind::Uninstalling => UiOperationKind::Uninstalling, + } +} + #[tauri::command] async fn update_game_directory(app_handle: tauri::AppHandle, path: String) -> tauri::Result<()> { log::info!("update_game_directory: {path}"); @@ -816,8 +876,16 @@ async fn handle_peer_event(app_handle: &AppHandle, event: PeerEvent) { log::info!("PeerEvent::ListGames received"); update_game_db(games, app_handle.clone()).await; } - PeerEvent::LocalGamesUpdated(local_games) => { + PeerEvent::LocalGamesUpdated { + games: local_games, + active_operations, + } => { log::info!("PeerEvent::LocalGamesUpdated received"); + { + let state = app_handle.state::(); + let mut ui_active_operations = state.active_operations.write().await; + reconcile_active_operations(&mut ui_active_operations, &active_operations); + } update_local_games_in_db(local_games, app_handle.clone()).await; } PeerEvent::GotGameFiles { @@ -1061,6 +1129,69 @@ async fn handle_download_finished(app_handle: &AppHandle, id: String) { .remove(&id); } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn active_operation_reconciliation_replaces_stale_ui_history() { + let mut active_operations = HashMap::from([ + ("stale".to_string(), UiOperationKind::Installing), + ("keep".to_string(), UiOperationKind::Downloading), + ]); + let snapshot = vec![ + ActiveOperation { + id: "keep".to_string(), + operation: ActiveOperationKind::Updating, + }, + ActiveOperation { + id: "new".to_string(), + operation: ActiveOperationKind::Uninstalling, + }, + ]; + + reconcile_active_operations(&mut active_operations, &snapshot); + + assert_eq!(active_operations.len(), 2); + assert_eq!( + active_operations.get("keep"), + Some(&UiOperationKind::Updating) + ); + assert_eq!( + active_operations.get("new"), + Some(&UiOperationKind::Uninstalling) + ); + assert!( + !active_operations.contains_key("stale"), + "snapshot reconciliation should drop stale UI operations" + ); + } + + #[test] + fn active_operation_payload_is_sorted_for_stable_ui_updates() { + let active_operations = HashMap::from([ + ("zeta".to_string(), UiOperationKind::Downloading), + ("alpha".to_string(), UiOperationKind::Installing), + ]); + + let snapshot = ui_active_operations_from_map(&active_operations); + + assert_eq!( + snapshot, + vec![ + UiActiveOperation { + id: "alpha".to_string(), + operation: UiOperationKind::Installing, + }, + UiActiveOperation { + id: "zeta".to_string(), + operation: UiOperationKind::Downloading, + }, + ] + ); + } +} + #[allow(clippy::missing_panics_doc)] #[cfg_attr(mobile, tauri::mobile_entry_point)] pub fn run() { diff --git a/crates/lanspread-tauri-deno-ts/src/App.tsx b/crates/lanspread-tauri-deno-ts/src/App.tsx index bbbd164..50af46c 100644 --- a/crates/lanspread-tauri-deno-ts/src/App.tsx +++ b/crates/lanspread-tauri-deno-ts/src/App.tsx @@ -55,6 +55,23 @@ enum GameAvailability { LocalOnly = 'LocalOnly', } +enum ActiveOperationKind { + Downloading = 'Downloading', + Installing = 'Installing', + Updating = 'Updating', + Uninstalling = 'Uninstalling', +} + +interface ActiveOperation { + id: string; + operation: ActiveOperationKind; +} + +interface GamesListPayload { + games: Game[]; + active_operations?: ActiveOperation[]; +} + interface GameThumbnailProps { gameId: string; alt: string; @@ -95,27 +112,78 @@ const IN_PROGRESS_INSTALL_STATUSES = new Set([ InstallStatus.Uninstalling, ]); +const RECONCILED_OPERATION_STATUSES = new Set([ + InstallStatus.Downloading, + InstallStatus.Installing, + InstallStatus.Uninstalling, +]); + const isInProgressInstallStatus = (status: InstallStatus): boolean => { return IN_PROGRESS_INSTALL_STATUSES.has(status); }; -const mergeGameUpdate = (game: Game, previous?: Game): Game => { +const isReconciledOperationStatus = (status: InstallStatus): boolean => { + return RECONCILED_OPERATION_STATUSES.has(status); +}; + +const installStatusFromActiveOperation = (operation: ActiveOperationKind): InstallStatus => { + switch (operation) { + case ActiveOperationKind.Downloading: + return InstallStatus.Downloading; + case ActiveOperationKind.Installing: + case ActiveOperationKind.Updating: + return InstallStatus.Installing; + case ActiveOperationKind.Uninstalling: + return InstallStatus.Uninstalling; + } +}; + +const activeStatusById = (activeOperations: ActiveOperation[] = []): Map => { + return new Map(activeOperations.map(operation => [ + operation.id, + installStatusFromActiveOperation(operation.operation), + ])); +}; + +const normalizeGamesListPayload = (payload: GamesListPayload | Game[]): GamesListPayload => { + if (Array.isArray(payload)) { + return { games: payload }; + } + return payload; +}; + +const mergeGameUpdate = ( + game: Game, + previous?: Game, + activeStatus?: InstallStatus, + hasAuthoritativeSnapshot = false, +): Game => { let installStatus = InstallStatus.NotInstalled; - if (game.installed) { + if (activeStatus !== undefined) { + installStatus = activeStatus; + } else if (game.installed) { installStatus = InstallStatus.Installed; - } else if (previous && isInProgressInstallStatus(previous.install_status)) { + } else if ( + previous + && isInProgressInstallStatus(previous.install_status) + && (!hasAuthoritativeSnapshot || previous.install_status === InstallStatus.CheckingPeers) + ) { installStatus = previous.install_status; } const localStateChanged = previous !== undefined && (previous.installed !== game.installed || previous.downloaded !== game.downloaded); + const activeStateReconciled = hasAuthoritativeSnapshot + && (activeStatus !== undefined + || (previous !== undefined && isReconciledOperationStatus(previous.install_status))); + const clearStatus = localStateChanged || activeStateReconciled; return { ...game, availability: game.availability ?? (game.downloaded ? GameAvailability.Ready : GameAvailability.LocalOnly), install_status: installStatus, - status_message: localStateChanged ? undefined : previous?.status_message, - status_level: localStateChanged ? undefined : previous?.status_level, + status_message: clearStatus ? undefined : previous?.status_message, + status_level: clearStatus ? undefined : previous?.status_level, peer_count: game.peer_count ?? 0, }; }; @@ -356,11 +424,19 @@ const App = () => { // Listen for games-list-updated events const unlisten_games = await listen('games-list-updated', (event) => { console.log('🗲 Received games-list-updated event'); - const games = event.payload as Game[]; + const payload = normalizeGamesListPayload(event.payload as GamesListPayload | Game[]); + const games = payload.games; + const activeStatuses = activeStatusById(payload.active_operations); + const hasAuthoritativeSnapshot = payload.active_operations !== undefined; console.log(`🎮 ${games.length} Games received`); setGameItems(prev => { const previousById = new Map(prev.map(item => [item.id, item])); - return games.map(game => mergeGameUpdate(game, previousById.get(game.id))); + return games.map(game => mergeGameUpdate( + game, + previousById.get(game.id), + activeStatuses.get(game.id), + hasAuthoritativeSnapshot, + )); }); void getInitialGameDir(); });