From 42dfe943cd8c5160b602bc4f72098f29487d2f0c Mon Sep 17 00:00:00 2001 From: Owen McGirr Date: Thu, 15 Oct 2026 12:15:30 +0100 Subject: [PATCH 1/2] Send update download progress on a small throttled event Each network chunk of an update download locked the shared state and emitted the whole AppState, running the diagnostics and telemetry observers, resetting the tray text and re-rendering the webview. A large installer meant thousands of these and could freeze the app. Bytes are now counted atomically outside the state mutex and sent on an update-progress event at most every 250 ms and only when the whole percent changes, with the completing sample always sent. Full state is emitted only for status transitions, with the final size at ready. The tray skips mutations whose value has not changed, and a downloading entry is recorded in the diagnostic history. Closes #1071 Co-Authored-By: Claude Opus 5.5 --- src-tauri/src/lib.rs | 111 ++++++++++++-- src-tauri/src/updater.rs | 259 ++++++++++++++++++++++++++++++-- src/App.tsx | 5 +- src/api.ts | 6 +- src/settings/UpdatesSection.tsx | 33 +++- src/settings/settings.test.tsx | 37 +++++ src/types.ts | 2 + 7 files changed, 419 insertions(+), 34 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 8c2e9406..40cac8e5 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -73,8 +73,8 @@ use tauri_plugin_autostart::ManagerExt as AutostartManagerExt; use tauri_plugin_updater::UpdaterExt; use telemetry::TelemetryConsent; use updater::{ - download_with_cancel, DownloadResult, Operation as UpdateOperation, RetryAction, - UpdateArtifact, UpdateManager, UpdateView, + download_with_cancel, DownloadResult, Operation as UpdateOperation, ProgressCounter, + ProgressThrottle, RetryAction, UpdateArtifact, UpdateManager, UpdateView, }; #[tauri::command] @@ -210,16 +210,55 @@ impl TraySnapshot { } } +/// The tray mutations a new snapshot needs. Each field is `None` when the +/// menu already shows that value, so unrelated state emissions leave the +/// native menu alone. +#[derive(Debug, Default, PartialEq, Eq)] +struct TrayChanges { + status: Option, + can_disconnect: Option, +} + +#[derive(Default)] +struct TrayCache(Mutex>); + +impl TrayCache { + fn changes(&self, next: TraySnapshot) -> TrayChanges { + let mut last = self + .0 + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let changes = match last.as_ref() { + Some(previous) => TrayChanges { + status: (previous.status != next.status).then(|| next.status.clone()), + can_disconnect: (previous.can_disconnect != next.can_disconnect) + .then_some(next.can_disconnect), + }, + None => TrayChanges { + status: Some(next.status.clone()), + can_disconnect: Some(next.can_disconnect), + }, + }; + *last = Some(next); + changes + } +} + struct TrayController { status: MenuItem, disconnect: MenuItem, + cache: TrayCache, } impl TrayController { fn sync(&self, state: &AppState) { - let snapshot = TraySnapshot::from_state(state); - let _ = self.status.set_text(snapshot.status); - let _ = self.disconnect.set_enabled(snapshot.can_disconnect); + let changes = self.cache.changes(TraySnapshot::from_state(state)); + if let Some(status) = changes.status { + let _ = self.status.set_text(status); + } + if let Some(can_disconnect) = changes.can_disconnect { + let _ = self.disconnect.set_enabled(can_disconnect); + } } } @@ -1167,25 +1206,28 @@ async fn download_update(app: AppHandle) -> Result { ActivityKind::Info, format!("Downloading Switchify PC {version}…"), ); + model.record_updater("downloading", None); publish_update(&app, &model, UpdateView::downloading(version.clone())); + // Progress runs per network chunk, so it stays off the shared state: + // bytes are counted atomically and only a throttled, small progress + // event reaches the webview. Full state is emitted on transitions only. + let counter = ProgressCounter::default(); + let mut throttle = ProgressThrottle::default(); let progress_app = app.clone(); - let progress_shared = model.shared.clone(); + let progress_counter = &counter; let result = download_with_cancel(&update, cancel_receiver, move |chunk, total| { - { - let mut data = progress_shared - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - data.state.updater.add_progress(chunk, total); + let progress = progress_counter.add(chunk, total); + if throttle.observe(std::time::Instant::now(), progress) { + let _ = progress_app.emit(updater::PROGRESS_EVENT, progress); } - state::emit_state(&progress_app, &progress_shared); }); let result = result.await; manager.finish(UpdateOperation::Download); match result { DownloadResult::Complete(Ok(bytes)) => { let downloaded_bytes = bytes.len() as u64; - let total_bytes = model.snapshot().updater.total_bytes; + let total_bytes = counter.current().total_bytes; manager.store_download(bytes); state::set_activity( &model.shared, @@ -1424,7 +1466,11 @@ fn install_tray(app: &mut tauri::App) -> tauri::Result<()> { &quit, ], )?; - let controller = TrayController { status, disconnect }; + let controller = TrayController { + status, + disconnect, + cache: TrayCache::default(), + }; controller.sync(&app.state::().snapshot()); app.manage(controller); let mut builder = TrayIconBuilder::with_id("switchify") @@ -1980,7 +2026,8 @@ mod tests { use super::{ account_pairing_intent, has_start_hidden_argument, record_update_failure, show_tray_menu_on_left_click, updater_is_configured, validate_profile, PendingNavigation, - PendingProfileExit, ProfileExitAction, TraySnapshot, NAVIGATE_REQUESTED_EVENT, + PendingProfileExit, ProfileExitAction, TrayCache, TrayChanges, TraySnapshot, + NAVIGATE_REQUESTED_EVENT, }; use crate::state::{AppModel, BluetoothState, SwitchBinding, SwitchProfile}; use crate::storage::AppStorage; @@ -2137,6 +2184,40 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + #[test] + fn tray_sync_only_mutates_values_that_changed() { + let snapshot = |status: &str, can_disconnect| TraySnapshot { + status: status.into(), + can_disconnect, + }; + let cache = TrayCache::default(); + assert_eq!( + cache.changes(snapshot("Status: Ready to connect", false)), + TrayChanges { + status: Some("Status: Ready to connect".into()), + can_disconnect: Some(false), + } + ); + assert_eq!( + cache.changes(snapshot("Status: Ready to connect", false)), + TrayChanges::default() + ); + assert_eq!( + cache.changes(snapshot("Status: Ready to connect", true)), + TrayChanges { + status: None, + can_disconnect: Some(true), + } + ); + assert_eq!( + cache.changes(snapshot("Status: Connected to Pixel", true)), + TrayChanges { + status: Some("Status: Connected to Pixel".into()), + can_disconnect: None, + } + ); + } + #[test] fn tray_navigation_uses_the_internal_navigation_event() { assert_eq!(NAVIGATE_REQUESTED_EVENT, "navigate-requested"); diff --git a/src-tauri/src/updater.rs b/src-tauri/src/updater.rs index cf9538c0..3923fc48 100644 --- a/src-tauri/src/updater.rs +++ b/src-tauri/src/updater.rs @@ -1,4 +1,12 @@ -use std::{future::Future, pin::Pin, sync::Mutex}; +use std::{ + future::Future, + pin::Pin, + sync::{ + atomic::{AtomicU64, Ordering}, + Mutex, + }, + time::{Duration, Instant}, +}; use serde::Serialize; use tauri_plugin_updater::Update; @@ -181,13 +189,6 @@ impl UpdateView { } } - pub fn add_progress(&mut self, chunk: usize, total: Option) { - self.downloaded_bytes = self.downloaded_bytes.saturating_add(chunk as u64); - if total.is_some() { - self.total_bytes = total; - } - } - fn new(status: UpdateStatus) -> Self { Self { status, @@ -200,6 +201,114 @@ impl UpdateView { } } +/// Tauri event carrying download progress. Progress travels on this small +/// event instead of the full `AppState` so a large download cannot flood +/// state emission, the tray and the webview. +pub const PROGRESS_EVENT: &str = "update-progress"; + +/// Minimum spacing between progress events. +pub const PROGRESS_INTERVAL: Duration = Duration::from_millis(250); + +#[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct UpdateProgress { + pub downloaded_bytes: u64, + pub total_bytes: Option, +} + +impl UpdateProgress { + fn percent(&self) -> Option { + self.total_bytes + .filter(|total| *total > 0) + .map(|total| (self.downloaded_bytes.min(total) * 100) / total) + } + + fn complete(&self) -> bool { + self.total_bytes + .is_some_and(|total| total > 0 && self.downloaded_bytes >= total) + } +} + +/// Byte counter shared with the download callback without touching the +/// application state mutex. A total of zero means the size is unknown. +#[derive(Debug, Default)] +pub struct ProgressCounter { + downloaded: AtomicU64, + total: AtomicU64, +} + +impl ProgressCounter { + pub fn add(&self, chunk: usize, total: Option) -> UpdateProgress { + if let Some(total) = total { + self.total.store(total, Ordering::Relaxed); + } + self.downloaded.fetch_add(chunk as u64, Ordering::Relaxed); + self.current() + } + + pub fn current(&self) -> UpdateProgress { + let total = self.total.load(Ordering::Relaxed); + UpdateProgress { + downloaded_bytes: self.downloaded.load(Ordering::Relaxed), + total_bytes: (total > 0).then_some(total), + } + } +} + +/// Decides which progress samples are worth emitting. After the first sample, +/// one is emitted only when the interval has elapsed since the last emission +/// and, when the total is known, the whole percentage has changed, so +/// whichever gate is less frequent wins. The completing sample is always +/// emitted, once, so the bar finishes promptly. +#[derive(Debug)] +pub struct ProgressThrottle { + interval: Duration, + last_at: Option, + last_percent: Option, + finished: bool, +} + +impl Default for ProgressThrottle { + fn default() -> Self { + Self::new(PROGRESS_INTERVAL) + } +} + +impl ProgressThrottle { + pub fn new(interval: Duration) -> Self { + Self { + interval, + last_at: None, + last_percent: None, + finished: false, + } + } + + pub fn observe(&mut self, now: Instant, progress: UpdateProgress) -> bool { + if self.finished { + return false; + } + let percent = progress.percent(); + let emit = if progress.complete() { + self.finished = true; + true + } else { + match self.last_at { + None => true, + Some(last) => { + now.saturating_duration_since(last) >= self.interval + && (percent.is_none() || percent != self.last_percent) + } + } + }; + if emit { + self.last_at = Some(now); + self.last_percent = percent; + } + emit + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Operation { Check, @@ -380,13 +489,137 @@ mod tests { } } + fn progress(downloaded_bytes: u64, total_bytes: Option) -> UpdateProgress { + UpdateProgress { + downloaded_bytes, + total_bytes, + } + } + + #[test] + fn progress_counter_accumulates_outside_the_state() { + let counter = ProgressCounter::default(); + assert_eq!(counter.current(), progress(0, None)); + assert_eq!(counter.add(25, Some(100)), progress(25, Some(100))); + assert_eq!(counter.add(30, None), progress(55, Some(100))); + } + + #[test] + fn progress_counter_reports_an_unknown_total() { + let counter = ProgressCounter::default(); + assert_eq!(counter.add(10, None), progress(10, None)); + } + + #[test] + fn throttle_waits_for_the_interval_even_when_the_percent_changes() { + let start = Instant::now(); + let ms = |value| start + Duration::from_millis(value); + let mut throttle = ProgressThrottle::default(); + assert!(throttle.observe(ms(0), progress(1, Some(100)))); + assert!(!throttle.observe(ms(100), progress(20, Some(100)))); + assert!(!throttle.observe(ms(249), progress(40, Some(100)))); + assert!(throttle.observe(ms(250), progress(41, Some(100)))); + assert!(!throttle.observe(ms(300), progress(60, Some(100)))); + } + + #[test] + fn throttle_waits_for_a_whole_percent_even_after_the_interval() { + let start = Instant::now(); + let ms = |value| start + Duration::from_millis(value); + let total = Some(10_000); + let mut throttle = ProgressThrottle::default(); + assert!(throttle.observe(ms(0), progress(100, total))); + assert!(!throttle.observe(ms(1_000), progress(150, total))); + assert!(!throttle.observe(ms(2_000), progress(199, total))); + assert!(throttle.observe(ms(3_000), progress(200, total))); + assert!(!throttle.observe(ms(4_000), progress(250, total))); + } + + #[test] + fn throttle_flushes_completion_once_regardless_of_timing() { + let start = Instant::now(); + let ms = |value| start + Duration::from_millis(value); + let mut throttle = ProgressThrottle::default(); + assert!(throttle.observe(ms(0), progress(99, Some(100)))); + assert!(throttle.observe(ms(10), progress(100, Some(100)))); + assert!(!throttle.observe(ms(1_000), progress(100, Some(100)))); + } + + #[test] + fn throttle_uses_time_alone_when_the_total_is_unknown() { + let start = Instant::now(); + let ms = |value| start + Duration::from_millis(value); + let mut throttle = ProgressThrottle::default(); + assert!(throttle.observe(ms(0), progress(10, None))); + assert!(!throttle.observe(ms(200), progress(20, None))); + assert!(throttle.observe(ms(250), progress(20, None))); + assert!(throttle.observe(ms(500), progress(30, None))); + assert!(!throttle.observe(ms(600), progress(40, None))); + } + + #[test] + fn throttle_bounds_events_for_a_large_download() { + let start = Instant::now(); + let total = 155 * 1024 * 1024_u64; + let chunk = 16 * 1024_u64; + let mut throttle = ProgressThrottle::default(); + let (mut downloaded, mut emitted, mut elapsed) = (0, 0, 0); + while downloaded < total { + downloaded = (downloaded + chunk).min(total); + elapsed += 1; + if throttle.observe( + start + Duration::from_millis(elapsed), + progress(downloaded, Some(total)), + ) { + emitted += 1; + } + } + assert!(emitted > 1); + assert!(emitted <= 101, "emitted {emitted} progress events"); + } + + #[tokio::test] + async fn fake_download_emits_throttled_progress_and_final_count() { + let update = FakeUpdate::successful(vec![vec![0; 10]; 50]); + let (_cancel, receiver) = watch::channel(false); + let counter = Arc::new(ProgressCounter::default()); + let events = Arc::new(Mutex::new(Vec::new())); + let (chunk_counter, chunk_events) = (counter.clone(), events.clone()); + let start = Instant::now(); + let mut throttle = ProgressThrottle::default(); + let result = download_with_cancel(&update, receiver, move |chunk, total| { + let sample = chunk_counter.add(chunk, total); + // Every chunk arrives at the same injected instant, so only the + // first and the completing samples pass. + if throttle.observe(start, sample) { + chunk_events + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .push(sample); + } + }) + .await; + assert!(matches!(result, DownloadResult::Complete(Ok(bytes)) if bytes.len() == 500)); + assert_eq!(counter.current(), progress(500, Some(500))); + assert_eq!( + *events + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()), + vec![progress(10, Some(500)), progress(500, Some(500))] + ); + } + + #[test] + fn progress_event_is_small_and_camel_cased() { + assert_eq!(PROGRESS_EVENT, "update-progress"); + assert_eq!( + serde_json::to_value(progress(5, Some(10))).unwrap(), + serde_json::json!({ "downloadedBytes": 5, "totalBytes": 10 }) + ); + } + #[test] fn transitions_expose_progress_retry_and_cancellation() { - let mut downloading = UpdateView::downloading("2.0.0".into()); - downloading.add_progress(25, Some(100)); - downloading.add_progress(30, None); - assert_eq!(downloading.downloaded_bytes, 55); - assert_eq!(downloading.total_bytes, Some(100)); assert_eq!( UpdateView::ready("2.0.0".into(), 100, Some(100)).status, UpdateStatus::ReadyToInstall diff --git a/src/App.tsx b/src/App.tsx index 2beca1f6..e20a78c5 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -12,7 +12,7 @@ import type { AccountView, AppSettings, AppState, PendingPairing, SwitchProfile, import { applyLocalSettings, changedSettingKeys } from "./settings/diff"; import { SettingsView } from "./settings/SettingsView"; import { MouseSettingsView } from "./settings/MouseSettingsView"; -import { updateDescription, updateInFlight, updateLiveness, updateProgress, updateStanding, type UpdateAction } from "./settings/UpdatesSection"; +import { updateDescription, updateInFlight, updateLiveness, updateProgress, updateStanding, useUpdateProgress, type UpdateAction } from "./settings/UpdatesSection"; import { TabPanel, Tabs } from "./Tabs"; import { useSwitches, type SwitchController } from "./scanning/useSwitches"; import { lastModeName, selectOpensHome, useScanning, type ScanningController } from "./scanning/useScanning"; @@ -336,7 +336,8 @@ function ProfilesView({ profiles, platform, saveProfile, deleteProfile, onDirtyC ; } -function UpdateBanner({ update, openUpdates }: { update: UpdateState; openUpdates: () => void }) { +function UpdateBanner({ update: state, openUpdates }: { update: UpdateState; openUpdates: () => void }) { + const update = useUpdateProgress(state); if (update.status !== "available" && update.status !== "downloading" && update.status !== "readyToInstall") return null; const message = update.status === "available" ? `Switchify PC ${update.version} is available.` diff --git a/src/api.ts b/src/api.ts index b76ec325..dda48656 100644 --- a/src/api.ts +++ b/src/api.ts @@ -1,6 +1,6 @@ import { invoke } from "@tauri-apps/api/core"; import { listen, type UnlistenFn } from "@tauri-apps/api/event"; -import type { AccountView, AppSettings, AppState, SwitchProfile, SyncView } from "./types"; +import type { AccountView, AppSettings, AppState, SwitchProfile, SyncView, UpdateProgress } from "./types"; export type ProfileExitAction = "hide" | "quit"; export type NavigationTarget = "home" | "settings" | "profiles"; @@ -146,6 +146,10 @@ export const api = { if (!("__TAURI_INTERNALS__" in window)) return () => undefined; return listen("app-state-changed", (event) => handler(event.payload)); }, + onUpdateProgress: async (handler: (progress: UpdateProgress) => void): Promise => { + if (!("__TAURI_INTERNALS__" in window)) return () => undefined; + return listen("update-progress", (event) => handler(event.payload)); + }, onProfileExitRequested: async (handler: (action: ProfileExitAction) => void): Promise => { if (!("__TAURI_INTERNALS__" in window)) return () => undefined; return listen("profile-exit-requested", (event) => handler(event.payload)); diff --git a/src/settings/UpdatesSection.tsx b/src/settings/UpdatesSection.tsx index ed57591d..946716f8 100644 --- a/src/settings/UpdatesSection.tsx +++ b/src/settings/UpdatesSection.tsx @@ -1,11 +1,37 @@ import { Button } from "../ui/controls"; import { Download, RefreshCw, X } from "lucide-react"; -import type { Ref } from "react"; -import type { AppState, UpdateState, UpdateStatus } from "../types"; +import { useEffect, useState, type Ref } from "react"; +import { api } from "../api"; +import type { AppState, UpdateProgress, UpdateState, UpdateStatus } from "../types"; import { SettingGroup } from "./controls"; export type UpdateAction = "check" | "download" | "install"; +// Download progress arrives on its own throttled event rather than in +// AppState, so only the components showing it re-render. AppState still +// carries the status transitions and the final size once ready. +export function useUpdateProgress(update: UpdateState): UpdateState { + const downloading = update.status === "downloading"; + const [progress, setProgress] = useState(null); + // Stay subscribed while mounted so the first samples of a download are not + // lost to a listener that is still registering. + useEffect(() => { + let disposed = false; + let unlisten: () => void = () => {}; + void api.onUpdateProgress((next) => { if (!disposed) setProgress(next); }).then((stop) => { + if (disposed) stop(); + else unlisten = stop; + }); + return () => { disposed = true; unlisten(); }; + }, []); + // A finished, failed or cancelled download leaves no stale progress behind. + useEffect(() => { + if (!downloading) return; + return () => setProgress(null); + }, [downloading, update.version]); + return downloading && progress ? { ...update, ...progress } : update; +} + export function updateProgress(update: UpdateState) { if (update.totalBytes && update.totalBytes > 0) { return `${Math.min(100, Math.round(update.downloadedBytes * 100 / update.totalBytes))}%`; @@ -51,7 +77,8 @@ export function updateInFlight(status: UpdateStatus) { return status === "checking"; } -export function UpdateControls({ update, run, cancel }: { update: UpdateState; run: (action: UpdateAction) => void; cancel: () => void }) { +export function UpdateControls({ update: state, run, cancel }: { update: UpdateState; run: (action: UpdateAction) => void; cancel: () => void }) { + const update = useUpdateProgress(state); const action = update.status === "available" || update.status === "cancelled" ? "download" : update.status === "readyToInstall" ? "install" : update.status === "failed" ? update.retryAction diff --git a/src/settings/settings.test.tsx b/src/settings/settings.test.tsx index 8354a3a6..2ef154be 100644 --- a/src/settings/settings.test.tsx +++ b/src/settings/settings.test.tsx @@ -83,6 +83,43 @@ describe("Switchify PC settings", () => { expect(cancel).toHaveBeenCalledOnce(); }); + it("follows the progress event during a download and keeps the final size from state", async () => { + let stateHandler: ((state: typeof browserState) => void) | undefined; + const progressHandlers = new Set<(progress: { downloadedBytes: number; totalBytes: number | null }) => void>(); + vi.spyOn(api, "onState").mockImplementation(async (handler) => { stateHandler = handler; return () => undefined; }); + const onProgress = vi.spyOn(api, "onUpdateProgress").mockImplementation(async (handler) => { + progressHandlers.add(handler); + return () => { progressHandlers.delete(handler); }; + }); + const sendProgress = (downloadedBytes: number, totalBytes: number | null) => + act(() => progressHandlers.forEach((handler) => handler({ downloadedBytes, totalBytes }))); + browserState.updater = { status: "downloading", version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: null }; + render(); + await screen.findByRole("heading", { name: "Switchify PC" }); + fireEvent.click(screen.getByRole("button", { name: "Settings" })); + selectTab("Updates"); + await waitFor(() => expect(progressHandlers.size).toBeGreaterThan(0)); + + sendProgress(1_000, null); + expect(document.querySelector(".update-controls > span")).toHaveTextContent(`${(1_000).toLocaleString()} bytes`); + sendProgress(150, 200); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "150"); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("max", "200"); + expect(document.querySelector(".update-controls > span")).toHaveTextContent("75%"); + expect(screen.getByRole("status", { name: "Application update" })).toHaveTextContent("Downloading Switchify PC 1.0.0-beta.2 — 75%"); + + act(() => stateHandler?.({ ...structuredClone(browserState), updater: { status: "readyToInstall", version: "1.0.0-beta.2", downloadedBytes: 200, totalBytes: 200, error: null, retryAction: null } })); + expect(screen.queryByRole("progressbar", { name: "Update download progress" })).toBeNull(); + expect(screen.getByText("Switchify PC 1.0.0-beta.2 is ready to install.", { selector: ".update-controls p" })).toBeInTheDocument(); + expect(onProgress).toHaveBeenCalled(); + + // A new download starts from its own state, not the previous attempt's samples. + act(() => stateHandler?.({ ...structuredClone(browserState), updater: { status: "downloading", version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: null } })); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "0"); + sendProgress(20, 200); + expect(document.querySelector(".update-controls > span")).toHaveTextContent("10%"); + }); + it("offers the correct retry action after a failure", async () => { browserState.updater = { status: "failed", version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: "Download failed", retryAction: "download" }; const download = vi.spyOn(api, "downloadUpdate").mockResolvedValue(structuredClone(browserState)); diff --git a/src/types.ts b/src/types.ts index 7a4bf930..24253901 100644 --- a/src/types.ts +++ b/src/types.ts @@ -18,6 +18,8 @@ export type UpdateState = { error: string | null; retryAction: "check" | "download" | "install" | null; }; +// Throttled byte counts sent on the `update-progress` event while downloading. +export type UpdateProgress = Pick; export type AppSettings = { startWithSystem: boolean; From 51a3fd23c33f30e31516c1c69c14914e51ca16c6 Mon Sep 17 00:00:00 2001 From: Owen McGirr Date: Thu, 15 Oct 2026 12:21:46 +0100 Subject: [PATCH 2/2] Keep one progress listener in App and retry failed tray updates The Updates tab mounted its own progress listener, so opening it during a download showed zero until the next throttled sample. App now owns the single listener and passes the merged updater state to the banner and the Updates tab. The tray cache recorded values before the native setters ran, so a failed call left the tray stale. Each value is now recorded only after its setter succeeds. Refs #1071 Co-Authored-By: Claude Opus 5.5 --- src-tauri/src/lib.rs | 138 ++++++++++++++++++++++---------- src/App.tsx | 10 ++- src/settings/SettingsView.tsx | 6 +- src/settings/UpdatesSection.tsx | 22 ++--- src/settings/settings.test.tsx | 68 ++++++++++++++-- 5 files changed, 177 insertions(+), 67 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 40cac8e5..a0fa8fc5 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -210,37 +210,46 @@ impl TraySnapshot { } } -/// The tray mutations a new snapshot needs. Each field is `None` when the -/// menu already shows that value, so unrelated state emissions leave the -/// native menu alone. +/// What the native tray menu is known to show. A field is recorded only after +/// its setter succeeds, so a failed call is retried on the next sync while +/// unchanged values leave the native menu alone. #[derive(Debug, Default, PartialEq, Eq)] -struct TrayChanges { +struct TrayShown { status: Option, can_disconnect: Option, } #[derive(Default)] -struct TrayCache(Mutex>); +struct TrayCache(Mutex); impl TrayCache { - fn changes(&self, next: TraySnapshot) -> TrayChanges { - let mut last = self - .0 + fn shown(&self) -> std::sync::MutexGuard<'_, TrayShown> { + self.0 .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - let changes = match last.as_ref() { - Some(previous) => TrayChanges { - status: (previous.status != next.status).then(|| next.status.clone()), - can_disconnect: (previous.can_disconnect != next.can_disconnect) - .then_some(next.can_disconnect), - }, - None => TrayChanges { - status: Some(next.status.clone()), - can_disconnect: Some(next.can_disconnect), - }, + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + + // The lock is not held across the setters: native menu calls may + // dispatch to the main thread. + fn apply( + &self, + next: TraySnapshot, + set_status: impl FnOnce(&str) -> Result<(), E>, + set_can_disconnect: impl FnOnce(bool) -> Result<(), E>, + ) { + let (status_changed, can_disconnect_changed) = { + let shown = self.shown(); + ( + shown.status.as_deref() != Some(next.status.as_str()), + shown.can_disconnect != Some(next.can_disconnect), + ) }; - *last = Some(next); - changes + if status_changed && set_status(&next.status).is_ok() { + self.shown().status = Some(next.status); + } + if can_disconnect_changed && set_can_disconnect(next.can_disconnect).is_ok() { + self.shown().can_disconnect = Some(next.can_disconnect); + } } } @@ -252,13 +261,11 @@ struct TrayController { impl TrayController { fn sync(&self, state: &AppState) { - let changes = self.cache.changes(TraySnapshot::from_state(state)); - if let Some(status) = changes.status { - let _ = self.status.set_text(status); - } - if let Some(can_disconnect) = changes.can_disconnect { - let _ = self.disconnect.set_enabled(can_disconnect); - } + self.cache.apply( + TraySnapshot::from_state(state), + |status| self.status.set_text(status), + |can_disconnect| self.disconnect.set_enabled(can_disconnect), + ); } } @@ -2026,7 +2033,7 @@ mod tests { use super::{ account_pairing_intent, has_start_hidden_argument, record_update_failure, show_tray_menu_on_left_click, updater_is_configured, validate_profile, PendingNavigation, - PendingProfileExit, ProfileExitAction, TrayCache, TrayChanges, TraySnapshot, + PendingProfileExit, ProfileExitAction, TrayCache, TrayShown, TraySnapshot, NAVIGATE_REQUESTED_EVENT, }; use crate::state::{AppModel, BluetoothState, SwitchBinding, SwitchProfile}; @@ -2191,29 +2198,76 @@ mod tests { can_disconnect, }; let cache = TrayCache::default(); + // Records the setter calls made for one sync; `fail` makes them error. + let sync = |next: TraySnapshot, fail: bool| { + let calls = std::cell::RefCell::new(Vec::new()); + let result = |call: String| { + calls.borrow_mut().push(call); + if fail { + Err(()) + } else { + Ok(()) + } + }; + cache.apply( + next, + |status| result(format!("text {status}")), + |enabled| result(format!("enabled {enabled}")), + ); + calls.into_inner() + }; assert_eq!( - cache.changes(snapshot("Status: Ready to connect", false)), - TrayChanges { - status: Some("Status: Ready to connect".into()), - can_disconnect: Some(false), - } + sync(snapshot("Status: Ready to connect", false), false), + ["text Status: Ready to connect", "enabled false"] ); + assert!(sync(snapshot("Status: Ready to connect", false), false).is_empty()); assert_eq!( - cache.changes(snapshot("Status: Ready to connect", false)), - TrayChanges::default() + sync(snapshot("Status: Ready to connect", true), false), + ["enabled true"] ); assert_eq!( - cache.changes(snapshot("Status: Ready to connect", true)), - TrayChanges { + sync(snapshot("Status: Connected to Pixel", true), false), + ["text Status: Connected to Pixel"] + ); + } + + #[test] + fn tray_sync_retries_a_value_whose_setter_failed() { + let snapshot = TraySnapshot { + status: "Status: Bluetooth is off".into(), + can_disconnect: true, + }; + let cache = TrayCache::default(); + let attempts = std::cell::Cell::new(0); + cache.apply( + snapshot.clone(), + |_| { + attempts.set(attempts.get() + 1); + Err(()) + }, + |_| Ok(()), + ); + assert_eq!( + *cache.shown(), + TrayShown { status: None, can_disconnect: Some(true), } ); + cache.apply( + snapshot.clone(), + |_| { + attempts.set(attempts.get() + 1); + Ok(()) + }, + |_| -> Result<(), ()> { panic!("unchanged value was set again") }, + ); + assert_eq!(attempts.get(), 2); assert_eq!( - cache.changes(snapshot("Status: Connected to Pixel", true)), - TrayChanges { - status: Some("Status: Connected to Pixel".into()), - can_disconnect: None, + *cache.shown(), + TrayShown { + status: Some(snapshot.status), + can_disconnect: Some(true), } ); } diff --git a/src/App.tsx b/src/App.tsx index e20a78c5..ffa45e8c 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -336,8 +336,7 @@ function ProfilesView({ profiles, platform, saveProfile, deleteProfile, onDirtyC ; } -function UpdateBanner({ update: state, openUpdates }: { update: UpdateState; openUpdates: () => void }) { - const update = useUpdateProgress(state); +function UpdateBanner({ update, openUpdates }: { update: UpdateState; openUpdates: () => void }) { if (update.status !== "available" && update.status !== "downloading" && update.status !== "readyToInstall") return null; const message = update.status === "available" ? `Switchify PC ${update.version} is available.` @@ -511,6 +510,9 @@ export function App() { const switches=useSwitches(); const scanning=useScanning(); const [state, setState] = useState(null); + // The one download-progress listener, owned here so the Updates tab shows + // the latest sample even when it mounts mid-download. + const liveUpdater = useUpdateProgress(state?.updater); const pairingOpen = !!state?.pendingPairings.length; const captureActive = switches.capturing || !!switches.state?.capture.active; const cancelCapture = useRef(switches.cancelCapture); @@ -818,7 +820,7 @@ export function App() { 0 || switches.capturing || !!switches.state?.capture.active}>
0}> {error &&
{error}
} - +

{updateNotice}

{view === "home" && void perform(api.disconnectAll)} onAccessibility={() => void perform(() => api.checkAccessibility(true))} />} {view === "switches" &&

Switches

The switches you use to control this computer.

} @@ -829,7 +831,7 @@ export function App() { {view === "devices" && void perform(() => api.forgetDevice(id))} />} {view === "profiles" && { profileEditorDirty.current = dirty; }} nativeExitRequest={profileExitRequest} onConfirmNativeExit={confirmProfileExit} onCancelNativeExit={cancelProfileExit} />} } - {view === "settings" && void perform(() => api.setTelemetryConsent(enabled))} updateAction={(action) => void runUpdate(action)} cancelUpdate={() => void cancelUpdate()} busy={busy} focusUpdates={focusUpdates} onUpdatesFocused={() => setFocusUpdates(false)} updateAttention={updateFailure?.text ?? null} onUpdatesShown={setUpdatesShown} />} + {view === "settings" && void perform(() => api.setTelemetryConsent(enabled))} updateAction={(action) => void runUpdate(action)} cancelUpdate={() => void cancelUpdate()} busy={busy} focusUpdates={focusUpdates} onUpdatesFocused={() => setFocusUpdates(false)} updateAttention={updateFailure?.text ?? null} onUpdatesShown={setUpdatesShown} />} {view === "account" &&

Account

Bring your settings to your other computers.

} {view === "support" && void perform(operation)} openSetup={openSetup} openUpdates={openUpdates} />}
diff --git a/src/settings/SettingsView.tsx b/src/settings/SettingsView.tsx index d962a558..5b149d44 100644 --- a/src/settings/SettingsView.tsx +++ b/src/settings/SettingsView.tsx @@ -1,5 +1,5 @@ import { useEffect, useMemo, useRef, useState } from "react"; -import type { AppSettings, AppState } from "../types"; +import type { AppSettings, AppState, UpdateState } from "../types"; import { Tabs, TabPanel, type TabDefinition } from "../Tabs"; import { GeneralSection } from "./GeneralSection"; import { InputSection } from "./InputSection"; @@ -10,7 +10,7 @@ import { UpdatesSection, type UpdateAction } from "./UpdatesSection"; type SettingsTabId = "general" | "input" | "cursor" | "privacy" | "updates"; -export function SettingsView({ state, settings, onChange, chooseTelemetry, updateAction, cancelUpdate, busy, focusUpdates, onUpdatesFocused, updateAttention, onUpdatesShown }: { state: AppState; settings: AppSettings; onChange: (next: AppSettings) => void; chooseTelemetry: (enabled: boolean) => void; updateAction: (action: UpdateAction) => void; cancelUpdate: () => void; busy: boolean; focusUpdates: boolean; onUpdatesFocused: () => void; updateAttention: string | null; onUpdatesShown: (shown: boolean) => void }) { +export function SettingsView({ state, updater, settings, onChange, chooseTelemetry, updateAction, cancelUpdate, busy, focusUpdates, onUpdatesFocused, updateAttention, onUpdatesShown }: { state: AppState; updater: UpdateState; settings: AppSettings; onChange: (next: AppSettings) => void; chooseTelemetry: (enabled: boolean) => void; updateAction: (action: UpdateAction) => void; cancelUpdate: () => void; busy: boolean; focusUpdates: boolean; onUpdatesFocused: () => void; updateAttention: string | null; onUpdatesShown: (shown: boolean) => void }) { const updatesRef = useRef(null); // Opening straight to Updates starts there, rather than committing General // for one frame and letting App announce a failure for a tab already being @@ -58,7 +58,7 @@ export function SettingsView({ state, settings, onChange, chooseTelemetry, updat {active === "input" && } {active === "cursor" && } {active === "privacy" && } - {active === "updates" && } + {active === "updates" && } ; } diff --git a/src/settings/UpdatesSection.tsx b/src/settings/UpdatesSection.tsx index 946716f8..cb9bbbd8 100644 --- a/src/settings/UpdatesSection.tsx +++ b/src/settings/UpdatesSection.tsx @@ -8,10 +8,12 @@ import { SettingGroup } from "./controls"; export type UpdateAction = "check" | "download" | "install"; // Download progress arrives on its own throttled event rather than in -// AppState, so only the components showing it re-render. AppState still -// carries the status transitions and the final size once ready. -export function useUpdateProgress(update: UpdateState): UpdateState { - const downloading = update.status === "downloading"; +// AppState, so a download no longer re-emits the whole state per chunk. +// AppState still carries the status transitions and the final size once +// ready. App owns the single listener so the latest sample survives views +// and tabs mounting and unmounting. +export function useUpdateProgress(update: UpdateState | undefined): UpdateState | undefined { + const downloading = update?.status === "downloading"; const [progress, setProgress] = useState(null); // Stay subscribed while mounted so the first samples of a download are not // lost to a listener that is still registering. @@ -28,8 +30,8 @@ export function useUpdateProgress(update: UpdateState): UpdateState { useEffect(() => { if (!downloading) return; return () => setProgress(null); - }, [downloading, update.version]); - return downloading && progress ? { ...update, ...progress } : update; + }, [downloading, update?.version]); + return update && downloading && progress ? { ...update, ...progress } : update; } export function updateProgress(update: UpdateState) { @@ -77,8 +79,7 @@ export function updateInFlight(status: UpdateStatus) { return status === "checking"; } -export function UpdateControls({ update: state, run, cancel }: { update: UpdateState; run: (action: UpdateAction) => void; cancel: () => void }) { - const update = useUpdateProgress(state); +export function UpdateControls({ update, run, cancel }: { update: UpdateState; run: (action: UpdateAction) => void; cancel: () => void }) { const action = update.status === "available" || update.status === "cancelled" ? "download" : update.status === "readyToInstall" ? "install" : update.status === "failed" ? update.retryAction @@ -96,6 +97,7 @@ export function UpdateControls({ update: state, run, cancel }: { update: UpdateS ; } -export function UpdatesSection({ state, run, cancel, sectionRef }: { state: AppState; run: (action: UpdateAction) => void; cancel: () => void; sectionRef: Ref }) { - return ; +// `update` is the app's updater state merged with the latest download progress. +export function UpdatesSection({ state, update, run, cancel, sectionRef }: { state: AppState; update: UpdateState; run: (action: UpdateAction) => void; cancel: () => void; sectionRef: Ref }) { + return ; } diff --git a/src/settings/settings.test.tsx b/src/settings/settings.test.tsx index 2ef154be..cf7a3458 100644 --- a/src/settings/settings.test.tsx +++ b/src/settings/settings.test.tsx @@ -83,7 +83,8 @@ describe("Switchify PC settings", () => { expect(cancel).toHaveBeenCalledOnce(); }); - it("follows the progress event during a download and keeps the final size from state", async () => { + // Fake state and progress events; the download itself never runs. + const fakeUpdateEvents = () => { let stateHandler: ((state: typeof browserState) => void) | undefined; const progressHandlers = new Set<(progress: { downloadedBytes: number; totalBytes: number | null }) => void>(); vi.spyOn(api, "onState").mockImplementation(async (handler) => { stateHandler = handler; return () => undefined; }); @@ -91,14 +92,65 @@ describe("Switchify PC settings", () => { progressHandlers.add(handler); return () => { progressHandlers.delete(handler); }; }); - const sendProgress = (downloadedBytes: number, totalBytes: number | null) => - act(() => progressHandlers.forEach((handler) => handler({ downloadedBytes, totalBytes }))); - browserState.updater = { status: "downloading", version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: null }; + return { + progressHandlers, + onProgress, + sendProgress: (downloadedBytes: number, totalBytes: number | null) => + act(() => progressHandlers.forEach((handler) => handler({ downloadedBytes, totalBytes }))), + sendUpdater: (updater: typeof browserState.updater) => + act(() => stateHandler?.({ ...structuredClone(browserState), updater })), + }; + }; + const downloadingUpdater = { status: "downloading" as const, version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: null }; + + it("shows the latest progress when Updates opens mid-download", async () => { + const { progressHandlers, sendProgress } = fakeUpdateEvents(); + browserState.updater = downloadingUpdater; + render(); + await screen.findByRole("heading", { name: "Switchify PC" }); + await waitFor(() => expect(progressHandlers.size).toBe(1)); + + sendProgress(150, 200); + expect(screen.getByRole("status", { name: "Application update" })).toHaveTextContent("Downloading Switchify PC 1.0.0-beta.2 — 75%"); + fireEvent.click(screen.getByRole("button", { name: "Settings" })); + selectTab("General"); + selectTab("Updates"); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "150"); + expect(document.querySelector(".update-controls > span")).toHaveTextContent("75%"); + expect(progressHandlers.size).toBe(1); + }); + + it.each([ + ["cancelled", { status: "cancelled" as const, version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: "download" as const }], + ["failed", { status: "failed" as const, version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: "Update download failed: offline", retryAction: "download" as const }], + ])("starts a retried download at zero after it was %s", async (_name, ended) => { + const { progressHandlers, sendProgress, sendUpdater } = fakeUpdateEvents(); + browserState.updater = downloadingUpdater; + render(); + await screen.findByRole("heading", { name: "Switchify PC" }); + fireEvent.click(screen.getByRole("button", { name: "Settings" })); + selectTab("Updates"); + await waitFor(() => expect(progressHandlers.size).toBe(1)); + sendProgress(150, 200); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "150"); + + sendUpdater(ended); + expect(screen.queryByRole("progressbar", { name: "Update download progress" })).toBeNull(); + sendUpdater(downloadingUpdater); + expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "0"); + expect(document.querySelector(".update-controls > span")).toHaveTextContent("0 bytes"); + sendProgress(20, 200); + expect(document.querySelector(".update-controls > span")).toHaveTextContent("10%"); + }); + + it("follows the progress event during a download and keeps the final size from state", async () => { + const { progressHandlers, onProgress, sendProgress, sendUpdater } = fakeUpdateEvents(); + browserState.updater = downloadingUpdater; render(); await screen.findByRole("heading", { name: "Switchify PC" }); fireEvent.click(screen.getByRole("button", { name: "Settings" })); selectTab("Updates"); - await waitFor(() => expect(progressHandlers.size).toBeGreaterThan(0)); + await waitFor(() => expect(progressHandlers.size).toBe(1)); sendProgress(1_000, null); expect(document.querySelector(".update-controls > span")).toHaveTextContent(`${(1_000).toLocaleString()} bytes`); @@ -108,13 +160,13 @@ describe("Switchify PC settings", () => { expect(document.querySelector(".update-controls > span")).toHaveTextContent("75%"); expect(screen.getByRole("status", { name: "Application update" })).toHaveTextContent("Downloading Switchify PC 1.0.0-beta.2 — 75%"); - act(() => stateHandler?.({ ...structuredClone(browserState), updater: { status: "readyToInstall", version: "1.0.0-beta.2", downloadedBytes: 200, totalBytes: 200, error: null, retryAction: null } })); + sendUpdater({ status: "readyToInstall", version: "1.0.0-beta.2", downloadedBytes: 200, totalBytes: 200, error: null, retryAction: null }); expect(screen.queryByRole("progressbar", { name: "Update download progress" })).toBeNull(); expect(screen.getByText("Switchify PC 1.0.0-beta.2 is ready to install.", { selector: ".update-controls p" })).toBeInTheDocument(); - expect(onProgress).toHaveBeenCalled(); + expect(onProgress).toHaveBeenCalledOnce(); // A new download starts from its own state, not the previous attempt's samples. - act(() => stateHandler?.({ ...structuredClone(browserState), updater: { status: "downloading", version: "1.0.0-beta.2", downloadedBytes: 0, totalBytes: null, error: null, retryAction: null } })); + sendUpdater(downloadingUpdater); expect(screen.getByRole("progressbar", { name: "Update download progress" })).toHaveAttribute("value", "0"); sendProgress(20, 200); expect(document.querySelector(".update-controls > span")).toHaveTextContent("10%");