From db66a995a5477b4889b258d48be5967fe4274a2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ca=C3=B1ete?= <2930882+juacker@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:46:49 +0200 Subject: [PATCH 1/3] feat(updates): make background downloads mandatory, drop the update toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating was three surfaces deep: a settings toggle, a dismissible toast, and the About panel. The toggle asked a question nobody wants asked (a downloaded package costs nothing and is discarded if never used), and the toast interrupted work to say something the top-bar pill was already saying. Now: detection stays as-is on every platform, the download is unconditional wherever the build can install updates itself, and the persistent pill grows a single "Restart to install" button once the package is on disk. Clicking it is still the only thing that installs an update — nothing restarts the app on its own. - Remove `AutoUpdateConfig` and the `get/set_auto_update_settings` commands; `AppUpdateStatus`/`AppUpdateCheckResult` no longer carry settings, and `get_app_update_status` is now infallible. - Configs on disk still carry `autoUpdate.autoDownload`; the field is ignored on load rather than rejected (regression test added). - Delete `AppUpdateNotifications` (toast) and its MainLayout wiring. - `AppUpdateBadge` keeps a stable "Update available · vX" label and adds the restart action when `downloaded`, with a visible, retryable error path so a failed install can never look like a dead click. --- src-tauri/src/commands/app_updates.rs | 76 +++------- src-tauri/src/config/mod.rs | 2 +- src-tauri/src/config/types.rs | 41 ++---- src-tauri/src/lib.rs | 2 - src/components/AppUpdateBadge.module.css | 45 ++++++ src/components/AppUpdateBadge.test.tsx | 131 +++++++++++++----- src/components/AppUpdateBadge.tsx | 95 ++++++++++--- .../AppUpdateNotifications.test.tsx | 111 --------------- src/components/AppUpdateNotifications.tsx | 121 ---------------- .../Settings/AboutSettings.module.css | 66 +-------- .../Settings/AboutSettings.test.tsx | 42 ++++-- src/components/Settings/AboutSettings.tsx | 68 +-------- src/generated/bindings.ts | 12 +- src/hooks/useAvailableAppUpdate.ts | 14 +- src/layouts/MainLayout.tsx | 2 - 15 files changed, 286 insertions(+), 542 deletions(-) delete mode 100644 src/components/AppUpdateNotifications.test.tsx delete mode 100644 src/components/AppUpdateNotifications.tsx diff --git a/src-tauri/src/commands/app_updates.rs b/src-tauri/src/commands/app_updates.rs index ff5b2c3b..cc9781a1 100644 --- a/src-tauri/src/commands/app_updates.rs +++ b/src-tauri/src/commands/app_updates.rs @@ -9,7 +9,6 @@ use tauri::ipc::Channel; use tauri::{AppHandle, Emitter, Manager, State}; use tauri_plugin_updater::{Error as UpdaterError, Update, UpdaterExt}; -use crate::config::AutoUpdateConfig; use crate::AppState; pub const APP_UPDATE_AVAILABLE_EVENT: &str = "app-updates://available"; @@ -153,7 +152,6 @@ pub struct AppUpdateLastCheck { #[serde(rename_all = "camelCase")] #[ts(export, export_to = "bindings.ts")] pub struct AppUpdateStatus { - pub settings: AutoUpdateConfig, pub support: AppUpdateSupportStatus, pub last_check: Option, } @@ -162,7 +160,6 @@ pub struct AppUpdateStatus { #[serde(rename_all = "camelCase")] #[ts(export, export_to = "bindings.ts")] pub struct AppUpdateCheckResult { - pub settings: AutoUpdateConfig, pub support: AppUpdateSupportStatus, pub last_check: AppUpdateLastCheck, } @@ -207,33 +204,11 @@ struct SupportProbe<'a> { } #[tauri::command] -pub fn get_auto_update_settings(state: State<'_, AppState>) -> Result { - auto_update_settings(state.inner()) -} - -#[tauri::command] -pub fn set_auto_update_settings( - settings: AutoUpdateConfig, - state: State<'_, AppState>, -) -> Result { - state - .config_manager - .lock() - .map_err(|e| format!("Config lock error: {e}"))? - .update(|config| { - config.auto_update = settings.clone(); - }) - .map_err(|e| format!("Failed to save update settings: {e}"))?; - Ok(settings) -} - -#[tauri::command] -pub fn get_app_update_status(state: State<'_, AppState>) -> Result { - Ok(AppUpdateStatus { - settings: auto_update_settings(state.inner())?, +pub fn get_app_update_status(state: State<'_, AppState>) -> AppUpdateStatus { + AppUpdateStatus { support: detect_support_status(), last_check: state.app_updates.last_check(), - }) + } } #[tauri::command] @@ -243,8 +218,7 @@ pub async fn check_for_app_update( ) -> Result { let result = check_for_update(&app, state.inner()).await; // Manual checks surface updates the same way startup checks do, so the - // persistent top-bar badge (and toast) appear no matter who found the - // update first. + // persistent top-bar badge appears no matter who found the update first. if let Some(update) = result.last_check.update.clone() { if let Err(error) = app.emit( APP_UPDATE_AVAILABLE_EVENT, @@ -257,7 +231,7 @@ pub async fn check_for_app_update( if update.installable && !update.downloaded { let app = app.clone(); tauri::async_runtime::spawn(async move { - auto_download_if_enabled(&app).await; + download_update_in_background(&app).await; }); } } @@ -339,8 +313,7 @@ pub fn spawn_startup_check(app: AppHandle) { }; // Checking is always on: it is a cheap, anonymous manifest fetch and - // the user must at least learn an update exists. Only the download - // is configurable (`AutoUpdateConfig::auto_download`). + // the user must at least learn an update exists. let result = check_for_update(&app, state.inner()).await; let Some(update) = result.last_check.update else { return; @@ -354,28 +327,24 @@ pub fn spawn_startup_check(app: AppHandle) { tracing::warn!(%error, "Failed to emit app update notification"); } if update.installable && !update.downloaded { - auto_download_if_enabled(&app).await; + download_update_in_background(&app).await; } }); } -/// Background download of an available update, gated on the -/// `autoUpdate.autoDownload` setting. Re-checks the updater manifest to get -/// a fresh signed package descriptor, downloads it, caches the bytes, and -/// re-emits the availability event with `downloaded: true` so the badge and -/// toast flip to \"restart to apply\". -async fn auto_download_if_enabled(app: &AppHandle) { +/// Background download of an available update. Unconditional on builds that +/// can install updates themselves: having the package ready is what lets the +/// UI offer a one-click "Restart to install" instead of a download wait, and +/// it costs the user nothing to decide later (or never). Installing is still +/// entirely the user's call — nothing here restarts the app. +/// +/// Re-checks the updater manifest to get a fresh signed package descriptor, +/// downloads it, caches the bytes, and re-emits the availability event with +/// `downloaded: true` so the badge grows its "Restart to install" action. +async fn download_update_in_background(app: &AppHandle) { let Some(state) = app.try_state::() else { return; }; - match auto_update_settings(state.inner()) { - Ok(settings) if settings.auto_download => {} - Ok(_) => return, - Err(error) => { - tracing::warn!(error, "Skipping update auto-download"); - return; - } - } if !detect_support_status().supported { return; } @@ -431,18 +400,8 @@ async fn auto_download_if_enabled(app: &AppHandle) { } } -fn auto_update_settings(state: &AppState) -> Result { - Ok(state - .config_manager - .lock() - .map_err(|e| format!("Config lock error: {e}"))? - .get() - .auto_update) -} - async fn check_for_update(app: &AppHandle, state: &AppState) -> AppUpdateCheckResult { let _check_guard = state.app_updates.check_lock.lock().await; - let settings = auto_update_settings(state).unwrap_or_default(); let support = detect_support_status(); let checked_at = chrono::Utc::now().to_rfc3339(); @@ -500,7 +459,6 @@ async fn check_for_update(app: &AppHandle, state: &AppState) -> AppUpdateCheckRe let last_check = state.app_updates.record_check(last_check); AppUpdateCheckResult { - settings, support, last_check, } diff --git a/src-tauri/src/config/mod.rs b/src-tauri/src/config/mod.rs index 11e2271c..9cd6c105 100644 --- a/src-tauri/src/config/mod.rs +++ b/src-tauri/src/config/mod.rs @@ -8,7 +8,7 @@ pub mod types; pub mod workspace_config; pub use types::{ - AgentConfig, AiProvider, AppConfig, AutoUpdateConfig, ClaiConfig, ExecutionCapabilityConfig, + AgentConfig, AiProvider, AppConfig, ClaiConfig, ExecutionCapabilityConfig, FilesystemPathAccess, FilesystemPathGrant, GrantOrigin, McpEnvVar, McpServerAuth, McpServerConfig, McpServerTransport, SandboxNetworkConfig, SandboxSessionBusConfig, ShellAccessMode, SkillSourceConfig, SkillSourceKind, diff --git a/src-tauri/src/config/types.rs b/src-tauri/src/config/types.rs index 2db68c38..387f6e8b 100644 --- a/src-tauri/src/config/types.rs +++ b/src-tauri/src/config/types.rs @@ -650,24 +650,6 @@ fn default_workspace_dirs() -> Vec { vec![PathBuf::from("~/.clai/workspaces")] } -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, ts_rs::TS)] -#[serde(rename_all = "camelCase", default)] -#[ts(export, export_to = "bindings.ts")] -pub struct AutoUpdateConfig { - /// Download new versions in the background on self-update-capable - /// builds; the user still chooses when to restart and apply. Checking - /// for updates is always on and not configurable. - pub auto_download: bool, -} - -impl Default for AutoUpdateConfig { - fn default() -> Self { - Self { - auto_download: true, - } - } -} - /// Root app configuration persisted at `~/.clai/config.json`. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -699,11 +681,6 @@ pub struct AppConfig { #[serde(default)] pub system_apps: crate::system_apps::SystemAppsConfig, - /// App updater preferences. Enabled by default for native installer builds; - /// runtime support detection gates package-manager-managed installs. - #[serde(default)] - pub auto_update: AutoUpdateConfig, - /// Global "pause all" overlay for the agent scheduler. When true, NO /// workspace's scheduled tick runs, regardless of its individual /// `schedule.paused` state — which is preserved underneath and restored @@ -723,7 +700,6 @@ impl Default for AppConfig { skill_sources: Vec::new(), provider_connections: Vec::new(), system_apps: crate::system_apps::SystemAppsConfig::default(), - auto_update: AutoUpdateConfig::default(), scheduler_paused: false, } } @@ -784,22 +760,21 @@ mod tests { } #[test] - fn app_config_defaults_auto_download_enabled() { - let config = ClaiConfig::default(); - assert!(config.auto_update.auto_download); - } - - #[test] - fn legacy_config_deserializes_with_auto_download_enabled() { + fn config_written_before_updates_became_mandatory_still_loads() { + // `autoUpdate.autoDownload` was a user setting until background + // downloads became mandatory on self-updating builds. Configs on + // disk still carry the key; loading must ignore it rather than + // failing and resetting the user's whole config to defaults. let legacy = r#"{ "version": 1, "workspaceDirs": ["~/.clai/workspaces"], "mcpServers": [], "skillSources": [], - "providerConnections": [] + "providerConnections": [], + "autoUpdate": { "autoDownload": false } }"#; let parsed: ClaiConfig = serde_json::from_str(legacy).unwrap(); - assert!(parsed.auto_update.auto_download); + assert_eq!(parsed.workspace_dirs, default_workspace_dirs()); } fn interval_kind(minutes: u32) -> crate::config::workspace_config::ScheduleKind { diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index c3435c77..145eca23 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -329,8 +329,6 @@ pub fn run() { // App metadata commands::app_info::app_version_detail, // App update commands - commands::app_updates::get_auto_update_settings, - commands::app_updates::set_auto_update_settings, commands::app_updates::get_app_update_status, commands::app_updates::check_for_app_update, commands::app_updates::install_app_update, diff --git a/src/components/AppUpdateBadge.module.css b/src/components/AppUpdateBadge.module.css index ea32bd10..00efb964 100644 --- a/src/components/AppUpdateBadge.module.css +++ b/src/components/AppUpdateBadge.module.css @@ -26,3 +26,48 @@ border-radius: 50%; background: currentColor; } + +.group { + display: inline-flex; + align-items: center; + gap: 8px; + flex-shrink: 0; + min-width: 0; +} + +/* Filled, unlike the outlined pill: the pill is information, this is the one + action that applies an update. */ +.action { + flex-shrink: 0; + padding: 4px 10px; + border: 1px solid var(--color-primary); + border-radius: 999px; + background: var(--color-primary); + color: var(--color-bg-primary); + font-size: 12px; + font-weight: 600; + line-height: 1; + cursor: pointer; + transition: opacity var(--transition-fast); +} + +.action:hover:not(:disabled) { + opacity: 0.85; +} + +.action:disabled { + cursor: default; + opacity: 0.6; +} + +/* A failed install must not be silent — the click produced no restart, so say + why. Truncated to protect the top-bar layout; the full text is the title. */ +.error { + max-width: 220px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + color: var(--color-error); + font-size: 12px; + line-height: 1; +} diff --git a/src/components/AppUpdateBadge.test.tsx b/src/components/AppUpdateBadge.test.tsx index c36b44c5..66239021 100644 --- a/src/components/AppUpdateBadge.test.tsx +++ b/src/components/AppUpdateBadge.test.tsx @@ -3,7 +3,14 @@ import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; const mockInvoke = vi.hoisted(() => vi.fn()); -vi.mock('@tauri-apps/api/core', () => ({ invoke: mockInvoke })); +vi.mock('@tauri-apps/api/core', () => ({ + invoke: mockInvoke, + // installAppUpdate hands the backend a progress Channel; the badge ignores + // progress (the package is already downloaded) but the class must exist. + Channel: class { + onmessage: unknown = null; + }, +})); // Capture the app-updates://available handler so tests can fire it. let listenHandlers: Record void> = {}; @@ -27,17 +34,16 @@ const UPDATE = { downloaded: false, }; -const LINUX_DEB_STATUS = { - settings: { autoDownload: true }, - support: { - supported: false, - canCheck: true, - platform: 'linux', - arch: 'x86_64', - channel: 'linux_pkg', - bundleType: 'deb', - reason: 'Notify only: updated by your package manager.', - }, +const DOWNLOADED = { ...UPDATE, downloaded: true }; + +const LINUX_DEB_SUPPORT = { + supported: false, + canCheck: true, + platform: 'linux', + arch: 'x86_64', + channel: 'linux_pkg', + bundleType: 'deb', + reason: 'Notify only: updated by your package manager.', }; const statusWith = ( @@ -52,11 +58,24 @@ const statusWith = ( reason: null, } ) => ({ - settings: { autoDownload: true }, support, lastCheck: update ? { checkedAt: '2026-07-24T12:00:00Z', update, error: null } : null, }); +/** Resolves `get_app_update_status`, and lets each test drive the install. */ +const mockBackend = ( + status: ReturnType, + install?: () => Promise +) => { + mockInvoke.mockImplementation((command: string) => { + if (command === 'get_app_update_status') return Promise.resolve(status); + if (command === 'install_app_update') { + return install ? install() : new Promise(() => {}); + } + return Promise.reject(new Error(`unexpected command ${command}`)); + }); +}; + beforeEach(() => { mockInvoke.mockReset(); listenHandlers = {}; @@ -64,63 +83,109 @@ beforeEach(() => { describe('AppUpdateBadge', () => { it('renders nothing when no update is available', async () => { - mockInvoke.mockResolvedValue(statusWith(null)); + mockBackend(statusWith(null)); const { container } = render(); await waitFor(() => expect(mockInvoke).toHaveBeenCalledWith('get_app_update_status')); expect(container).toBeEmptyDOMElement(); }); it('shows the version from the seeded backend status', async () => { - mockInvoke.mockResolvedValue(statusWith(UPDATE)); + mockBackend(statusWith(UPDATE)); render(); expect(await screen.findByText(/Update available · v26\.8\.1/)).toBeInTheDocument(); }); it('still renders when the build is notify-only (Linux deb/rpm)', async () => { // The badge shows the same "Update available" pill for notify-only - // builds as for self-updating ones — the only difference is that - // About shows the package-manager copy and there is no toast. - const notifyOnlyUpdate = { ...UPDATE, installable: false }; - mockInvoke.mockResolvedValue(statusWith(notifyOnlyUpdate, LINUX_DEB_STATUS.support)); + // builds as for self-updating ones — the difference is that no package + // is ever downloaded, so the restart action never appears. + mockBackend(statusWith({ ...UPDATE, installable: false }, LINUX_DEB_SUPPORT)); render(); expect(await screen.findByText(/Update available · v26\.8\.1/)).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /Restart to install/ })).not.toBeInTheDocument(); }); it('appears when an update event fires after mount', async () => { - mockInvoke.mockResolvedValue(statusWith(null)); + mockBackend(statusWith(null)); render(); await waitFor(() => expect(listenHandlers[APP_UPDATE_AVAILABLE_EVENT]).toBeDefined()); listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ payload: { update: UPDATE } }); expect(await screen.findByText(/Update available · v26\.8\.1/)).toBeInTheDocument(); }); - it('flips to "Update ready" once the package is downloaded', async () => { - mockInvoke.mockResolvedValue(statusWith(UPDATE)); + it('offers the restart action only once the package is downloaded', async () => { + mockBackend(statusWith(UPDATE)); render(); expect(await screen.findByText(/Update available · v26\.8\.1/)).toBeInTheDocument(); + // While the background download runs there is nothing to restart into, + // so the pill is informational only. + expect(screen.queryByRole('button', { name: /Restart to install/ })).not.toBeInTheDocument(); + await waitFor(() => expect(listenHandlers[APP_UPDATE_AVAILABLE_EVENT]).toBeDefined()); - listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ - payload: { update: { ...UPDATE, downloaded: true } }, - }); - expect(await screen.findByText(/Update ready · v26\.8\.1/)).toBeInTheDocument(); + listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ payload: { update: DOWNLOADED } }); + expect( + await screen.findByRole('button', { name: /Restart to install/ }) + ).toBeInTheDocument(); + // The pill copy stays stable across the flip: the new button is the + // signal, so the version label never rewrites itself under the cursor. + expect(screen.getByText(/Update available · v26\.8\.1/)).toBeInTheDocument(); }); - it('does not downgrade "Update ready" when a stale event arrives late', async () => { - mockInvoke.mockResolvedValue(statusWith(null)); + it('installs and restarts only when the restart action is clicked', async () => { + mockBackend(statusWith(DOWNLOADED)); render(); + const action = await screen.findByRole('button', { name: /Restart to install/ }); + expect(mockInvoke).not.toHaveBeenCalledWith('install_app_update', expect.anything()); + + await userEvent.click(action); + + expect(mockInvoke).toHaveBeenCalledWith('install_app_update', expect.anything()); + // The backend restarts the app, so the pending state must stay pending + // rather than inviting a second click into the same install. + expect(await screen.findByRole('button', { name: /Restarting/ })).toBeDisabled(); + }); + + it('surfaces a failed install instead of silently doing nothing', async () => { + mockBackend(statusWith(DOWNLOADED), () => Promise.reject('Permission denied')); + render(); + await userEvent.click(await screen.findByRole('button', { name: /Restart to install/ })); + + expect(await screen.findByRole('alert')).toHaveTextContent('Permission denied'); + // Re-enabled: a failed install must be retryable. + expect(screen.getByRole('button', { name: /Restart to install/ })).toBeEnabled(); + }); + + it('clears a failed install when a newer version arrives', async () => { + mockBackend(statusWith(DOWNLOADED), () => Promise.reject('Permission denied')); + render(); + await userEvent.click(await screen.findByRole('button', { name: /Restart to install/ })); + expect(await screen.findByRole('alert')).toBeInTheDocument(); + await waitFor(() => expect(listenHandlers[APP_UPDATE_AVAILABLE_EVENT]).toBeDefined()); listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ - payload: { update: { ...UPDATE, downloaded: true } }, + payload: { update: { ...DOWNLOADED, version: '26.8.2' } }, }); - expect(await screen.findByText(/Update ready · v26\.8\.1/)).toBeInTheDocument(); + + expect(await screen.findByText(/Update available · v26\.8\.2/)).toBeInTheDocument(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('does not downgrade a downloaded update when a stale event arrives late', async () => { + mockBackend(statusWith(null)); + render(); + await waitFor(() => expect(listenHandlers[APP_UPDATE_AVAILABLE_EVENT]).toBeDefined()); + listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ payload: { update: DOWNLOADED } }); + expect( + await screen.findByRole('button', { name: /Restart to install/ }) + ).toBeInTheDocument(); // A concurrent check that started before the download finished can emit - // downloaded: false after the fact — the badge must not regress. + // downloaded: false after the fact — the action must not vanish. listenHandlers[APP_UPDATE_AVAILABLE_EVENT]?.({ payload: { update: UPDATE } }); - expect(await screen.findByText(/Update ready · v26\.8\.1/)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Restart to install/ })).toBeInTheDocument(); }); it('opens global settings at the About tab on click', async () => { - mockInvoke.mockResolvedValue(statusWith(UPDATE)); + mockBackend(statusWith(UPDATE)); const opened = vi.fn(); const onOpen = (event: Event) => opened((event as CustomEvent).detail); window.addEventListener(OPEN_GLOBAL_SETTINGS_EVENT, onOpen); diff --git a/src/components/AppUpdateBadge.tsx b/src/components/AppUpdateBadge.tsx index b6a7c609..6e440fda 100644 --- a/src/components/AppUpdateBadge.tsx +++ b/src/components/AppUpdateBadge.tsx @@ -1,40 +1,93 @@ /** * AppUpdateBadge Component * - * Persistent "update available" pill for the fleet top bar. Unlike the - * dismissible toast (AppUpdateNotifications), this stays visible until the - * update is actually applied, so the user is always aware a new version - * exists. Clicking it opens the global Settings modal at the About section, - * which hosts the full install / view-release controls. + * The app's only update surface: a persistent pill in the fleet top bar, + * visible from every route (FleetLayout wraps both `/fleet` and + * `/workspace/:id`). It stays up until the update is applied — deliberately + * not dismissible, and deliberately not a toast: an update is worth knowing + * about, never worth interrupting for. + * + * Clicking the pill opens Settings > About, which hosts the release notes, + * the manual check and the notify-only (Linux package manager) copy. + * + * Once the package has finished downloading in the background, a "Restart to + * install" button appears beside the pill. That click is the ONLY thing that + * installs an update; the package is already on disk, so it is a restart, not + * a download wait. Ignoring it costs the user nothing — the app keeps running + * the current version and picks the download up again on the next launch. */ -import React from 'react'; +import React, { useCallback, useState } from 'react'; import { useAvailableAppUpdate } from '../hooks/useAvailableAppUpdate'; import { openGlobalSettings } from '../utils/globalSettings'; +import { installAppUpdate, updateErrorText } from '../utils/appUpdates'; import styles from './AppUpdateBadge.module.css'; const AppUpdateBadge = () => { const { update } = useAvailableAppUpdate(); + const [restarting, setRestarting] = useState(false); + const [error, setError] = useState(''); + + // A newer version arriving must not inherit the previous version's failed + // attempt. Render-phase state adjustment (React's documented pattern for + // derived resets) keeps the reset in the same commit as the new version. + const version = update?.version ?? null; + const [seenVersion, setSeenVersion] = useState(version); + if (version !== seenVersion) { + setSeenVersion(version); + setRestarting(false); + setError(''); + } + + const restartToInstall = useCallback(async () => { + setRestarting(true); + setError(''); + try { + // The backend installs the cached package and restarts the app, so a + // resolved promise is not the success path — the process is gone by + // then. Only the rejection matters here. + await installAppUpdate(() => {}); + setRestarting(false); + } catch (err) { + setError(updateErrorText(err, 'Could not install the update.')); + setRestarting(false); + } + }, []); if (!update) return null; return ( - + } + aria-label={`Update available: CLAI v${update.version}`} + > + - {/* Checking for updates is always on; only the background download is - configurable, and only where this build can actually install - updates itself. Notify-only builds (Flatpak, Linux deb/rpm) get - no toggle — there is nothing to install from within CLAI. */} - {supportsUpdates && ( - - )} -
{updateSummary} {updateError && {updateError}} diff --git a/src/generated/bindings.ts b/src/generated/bindings.ts index 92b257b0..68cb49de 100644 --- a/src/generated/bindings.ts +++ b/src/generated/bindings.ts @@ -5,7 +5,7 @@ export type AddSkillSourceRequest = { name: string, kind: string | null, path: s export type AppUpdateAvailableEvent = { update: AppUpdateInfo, }; -export type AppUpdateCheckResult = { settings: AutoUpdateConfig, support: AppUpdateSupportStatus, lastCheck: AppUpdateLastCheck, }; +export type AppUpdateCheckResult = { support: AppUpdateSupportStatus, lastCheck: AppUpdateLastCheck, }; export type AppUpdateInfo = { currentVersion: string, version: string, date: string | null, body: string | null, /** @@ -24,7 +24,7 @@ export type AppUpdateInstallEvent = { "type": "started" } | { "type": "progress" export type AppUpdateLastCheck = { checkedAt: string, update: AppUpdateInfo | null, error: string | null, }; -export type AppUpdateStatus = { settings: AutoUpdateConfig, support: AppUpdateSupportStatus, lastCheck: AppUpdateLastCheck | null, }; +export type AppUpdateStatus = { support: AppUpdateSupportStatus, lastCheck: AppUpdateLastCheck | null, }; export type AppUpdateSupportStatus = { /** @@ -77,14 +77,6 @@ export type AttentionUpdate = { workspaceId: string | null, pendingCount: number export type AuthMode = "subscription_login" | "subscription_api_key" | "developer_api_key" | "workspace_token"; -export type AutoUpdateConfig = { -/** - * Download new versions in the background on self-update-capable - * builds; the user still chooses when to restart and apply. Checking - * for updates is always on and not configurable. - */ -autoDownload: boolean, }; - export type CompactionStatus = "running" | "completed" | "failed"; export type CompactionStrategy = "local_summary" | "session_rotation_summary"; diff --git a/src/hooks/useAvailableAppUpdate.ts b/src/hooks/useAvailableAppUpdate.ts index 48a00fc6..512207f8 100644 --- a/src/hooks/useAvailableAppUpdate.ts +++ b/src/hooks/useAvailableAppUpdate.ts @@ -3,15 +3,13 @@ * * Seeds from the backend's last check result (so a UI mounted after the * startup check still sees the update) and then follows the - * `app-updates://available` event emitted by later checks. Used by both - * the dismissible toast (AppUpdateNotifications) and the persistent - * top-bar badge (AppUpdateBadge) so they can't drift apart. + * `app-updates://available` event emitted by later checks. Consumed by the + * persistent top-bar badge (AppUpdateBadge) and by Settings > About. * - * Also exposes the build's `support` profile so consumers can adapt to - * the host's update capability (e.g. suppress the toast on Linux, - * which is always notify-only). Support is a build/install-time - * property and is read once from the initial status; the live event - * does not carry it. + * Also exposes the build's `support` profile so consumers can adapt to the + * host's update capability (e.g. Linux, which is always notify-only). + * Support is a build/install-time property and is read once from the initial + * status; the live event does not carry it. */ import { useEffect, useState } from 'react'; diff --git a/src/layouts/MainLayout.tsx b/src/layouts/MainLayout.tsx index 3454b4a9..3612c650 100644 --- a/src/layouts/MainLayout.tsx +++ b/src/layouts/MainLayout.tsx @@ -1,6 +1,5 @@ import React, { useEffect } from 'react'; import { Outlet } from 'react-router'; -import AppUpdateNotifications from '../components/AppUpdateNotifications'; import TerminalEmulatorWrapper from '../components/TerminalEmulator/TerminalEmulatorWrapper'; import PermissionAttentionNotifications from '../components/PermissionAttentionNotifications'; import WorkspaceTaskNotifications from '../components/WorkspaceTaskNotifications'; @@ -25,7 +24,6 @@ const MainLayout = () => {
-
From 7cb58a3be069560f7430d62977a3a993b095aa39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ca=C3=B1ete?= <2930882+juacker@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:05:59 +0200 Subject: [PATCH 2/3] fix(updates): apply the downloaded package without a network round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit found that "Restart to install" did not actually restart: `install_app_update` fetched the manifest again before looking at the cache, so the click failed outright when offline, and if a newer release had appeared meanwhile the matching-version cache was dropped and the click silently became a multi-minute download behind a disabled "Restarting..." with no progress. That was survivable while the toast offered progress; it is not survivable now that the button is the only install surface. The updater's `Update` handle is `Clone` and carries everything `install` needs, so the package cache now keeps the handle with the bytes it produced. Applying it touches no network at all, and installs exactly the version the badge advertised. The manifest check moves into the no-cache branch, where Settings > About still downloads with progress. Also from review: - A failed install now always keeps its verified package (a fresh download used to be discarded), via one `cache_downloaded_package` helper shared with the background download so both leave the same state behind. - The mandatory-download rule is now a pure `should_download_in_background` predicate with tests that fail if a setting is ever wired back in — the property had no Rust coverage at all. - The config back-compat test asserted values identical to the defaults, so it could not fail. It now carries non-default settings and a negative control was run (adding `deny_unknown_fields` makes it fail). - The install error left the top bar: it is the button's own label plus a tooltip and an `sr-only` live region, so an arbitrarily long message can no longer stretch the bar. `.counters` shrinks and clips instead of pushing the update action and settings button off-screen. --- src-tauri/src/commands/app_updates.rs | 260 ++++++++++++++---- src-tauri/src/config/types.rs | 24 +- src/components/AppUpdateBadge.module.css | 17 +- src/components/AppUpdateBadge.test.tsx | 7 +- src/components/AppUpdateBadge.tsx | 22 +- .../Settings/AboutSettings.test.tsx | 6 +- src/components/Settings/AboutSettings.tsx | 2 +- src/layouts/FleetLayout.module.css | 7 + 8 files changed, 252 insertions(+), 93 deletions(-) diff --git a/src-tauri/src/commands/app_updates.rs b/src-tauri/src/commands/app_updates.rs index cc9781a1..e41e8ba4 100644 --- a/src-tauri/src/commands/app_updates.rs +++ b/src-tauri/src/commands/app_updates.rs @@ -27,20 +27,55 @@ const LATEST_MANIFEST_URL: &str = /// Update package downloaded in the background, waiting for the user to /// restart. Kept in memory: packages are tens of MB and the alternative /// (a temp file) would need cleanup and re-verification on install. -struct DownloadedPackage { +/// +/// `installer` is the updater handle that produced `bytes`, kept alongside +/// them. It is what makes "Restart to install" a restart rather than a +/// download: applying the package needs no network, so the click cannot be +/// derailed by an offline machine, by a slow manifest fetch, or by a newer +/// release appearing between the download and the click. +struct DownloadedPackage { version: String, + installer: I, bytes: Vec, } -#[derive(Clone, Default)] -pub struct AppUpdateRuntime { +/// The one downloaded package we hold, if any. A newer download replaces an +/// older one: the badge only ever advertises a single version. +struct PackageCache { + package: Option>, +} + +impl Default for PackageCache { + fn default() -> Self { + Self { package: None } + } +} + +/// In-memory update state for the running app. +/// +/// Generic over the installer type purely as a test seam: production always +/// uses `Update`, which cannot be constructed outside the updater plugin, so +/// tests instantiate `AppUpdateRuntime<()>` to exercise the bookkeeping. +#[derive(Clone)] +pub struct AppUpdateRuntime { last_check: Arc>>, - downloaded: Arc>>, + downloaded: Arc>>, check_lock: Arc>, install_lock: Arc>, } -impl AppUpdateRuntime { +impl Default for AppUpdateRuntime { + fn default() -> Self { + Self { + last_check: Arc::default(), + downloaded: Arc::default(), + check_lock: Arc::default(), + install_lock: Arc::default(), + } + } +} + +impl AppUpdateRuntime { pub fn new() -> Self { Self::default() } @@ -71,25 +106,32 @@ impl AppUpdateRuntime { self.downloaded .lock() .expect("app update state poisoned") + .package .as_ref() .map(|package| package.version.clone()) } - fn store_downloaded(&self, version: &str, bytes: Vec) { - *self.downloaded.lock().expect("app update state poisoned") = Some(DownloadedPackage { + fn store_downloaded(&self, version: &str, installer: I, bytes: Vec) { + self.downloaded + .lock() + .expect("app update state poisoned") + .package = Some(DownloadedPackage { version: version.to_string(), + installer, bytes, }); } - /// Takes the cached package if it matches `version`; a mismatch (a newer - /// release appeared since the download) drops the stale cache instead. - fn take_downloaded(&self, version: &str) -> Option> { - let mut guard = self.downloaded.lock().expect("app update state poisoned"); - match guard.take() { - Some(package) if package.version == version => Some(package.bytes), - _ => None, - } + /// Takes the downloaded package, whatever version it is. Installing + /// exactly what was downloaded is the point: it is the version the badge + /// offered to install, and it needs no network to apply. A newer release + /// is picked up by the next check, which replaces the cache. + fn take_downloaded(&self) -> Option> { + self.downloaded + .lock() + .expect("app update state poisoned") + .package + .take() } /// Flags the recorded last check's update as downloaded (if it is still @@ -203,6 +245,15 @@ struct SupportProbe<'a> { has_rpm: bool, } +/// Whether an available update should be downloaded in the background. +/// +/// Mandatory by design — no user setting gates this — but only where the +/// build can install what it downloads: notify-only channels (Flatpak, Linux +/// deb/rpm) would burn bandwidth on a package they can never apply. +fn should_download_in_background(support: &AppUpdateSupportStatus, update: &AppUpdateInfo) -> bool { + support.supported && update.installable && !update.downloaded +} + #[tauri::command] pub fn get_app_update_status(state: State<'_, AppState>) -> AppUpdateStatus { AppUpdateStatus { @@ -228,7 +279,7 @@ pub async fn check_for_app_update( ) { tracing::warn!(%error, "Failed to emit app update notification"); } - if update.installable && !update.downloaded { + if should_download_in_background(&result.support, &update) { let app = app.clone(); tauri::async_runtime::spawn(async move { download_update_in_background(&app).await; @@ -252,26 +303,30 @@ pub async fn install_app_update( .unwrap_or_else(|| "This CLAI build cannot update itself.".to_string())); } - let update = app - .updater_builder() - .timeout(INSTALL_TIMEOUT) - .build() - .map_err(format_updater_error)? - .check() - .await - .map_err(format_updater_error)? - .ok_or_else(|| "No update is available.".to_string())?; - let _ = on_event.send(AppUpdateInstallEvent::Started); - // Reuse a package the background auto-download already fetched (and the - // updater plugin signature-verified) when it matches the version we are - // about to install; otherwise download it now. - let (bytes, from_cache) = match state.app_updates.take_downloaded(&update.version) { - Some(bytes) => { + // Fast path: the background download already fetched this package and the + // updater plugin verified its signature, and the handle it came from is + // cached with it. So there is nothing left to fetch — no manifest check, + // no network at all — which is what lets the badge promise a restart + // rather than a wait, offline included. + let package = match state.app_updates.take_downloaded() { + Some(package) => { let _ = on_event.send(AppUpdateInstallEvent::DownloadFinished); - (bytes, true) + package } + // Nothing cached: the background download is still running, failed, + // or never started (Settings > About can ask for an install the + // moment a check reports one). Fetch it now, reporting progress. None => { + let update = app + .updater_builder() + .timeout(INSTALL_TIMEOUT) + .build() + .map_err(format_updater_error)? + .check() + .await + .map_err(format_updater_error)? + .ok_or_else(|| "No update is available.".to_string())?; let mut downloaded: u64 = 0; let bytes = tokio::time::timeout( DOWNLOAD_TIMEOUT, @@ -289,17 +344,19 @@ pub async fn install_app_update( .await .map_err(|_| "Timed out downloading the update package.".to_string())? .map_err(format_updater_error)?; - (bytes, false) + DownloadedPackage { + version: update.version.clone(), + installer: update, + bytes, + } } }; let _ = on_event.send(AppUpdateInstallEvent::Installing); - if let Err(error) = update.install(&bytes) { - // Put a cached package back so the "downloaded — restart to apply" - // state stays truthful and a retry does not silently re-download. - if from_cache { - state.app_updates.store_downloaded(&update.version, bytes); - } + if let Err(error) = package.installer.install(&package.bytes) { + // Keep the verified package: the retry is then a restart instead of + // another download, and the badge's "downloaded" state stays true. + cache_downloaded_package(&app, state.inner(), package); return Err(format_updater_error(error)); } app.restart(); @@ -326,7 +383,7 @@ pub fn spawn_startup_check(app: AppHandle) { ) { tracing::warn!(%error, "Failed to emit app update notification"); } - if update.installable && !update.downloaded { + if should_download_in_background(&result.support, &update) { download_update_in_background(&app).await; } }); @@ -345,9 +402,6 @@ async fn download_update_in_background(app: &AppHandle) { let Some(state) = app.try_state::() else { return; }; - if !detect_support_status().supported { - return; - } // Serialize with manual installs; whichever runs first downloads. let _install_guard = state.app_updates.install_lock.lock().await; @@ -388,9 +442,29 @@ async fn download_update_in_background(app: &AppHandle) { } }; - state.app_updates.store_downloaded(&update.version, bytes); tracing::info!(version = %update.version, "Update downloaded in the background"); - if let Some(update) = state.app_updates.mark_downloaded(&update.version) { + cache_downloaded_package( + app, + state.inner(), + DownloadedPackage { + version: update.version.clone(), + installer: update, + bytes, + }, + ); +} + +/// Holds a verified package and tells the UI it can offer "Restart to +/// install". Shared by the background download and by a failed install +/// putting its package back, so both leave the same state behind. +fn cache_downloaded_package(app: &AppHandle, state: &AppState, package: DownloadedPackage) { + let version = package.version.clone(); + state + .app_updates + .store_downloaded(&version, package.installer, package.bytes); + // Only emits when the recorded check still names this version; a check + // that has already moved on owns the badge's state instead. + if let Some(update) = state.app_updates.mark_downloaded(&version) { if let Err(error) = app.emit( APP_UPDATE_AVAILABLE_EVENT, AppUpdateAvailableEvent { update }, @@ -927,29 +1001,95 @@ mod tests { } } + /// A real `Update` cannot be built outside the updater plugin, so the + /// bookkeeping is exercised with `()` standing in for the installer. + fn runtime() -> AppUpdateRuntime<()> { + AppUpdateRuntime::new() + } + + fn supported() -> AppUpdateSupportStatus { + super::supported(probe("macos", Some("app")), "native") + } + + #[test] + fn background_download_is_mandatory_on_self_updating_builds() { + // The download used to be opt-out via `autoUpdate.autoDownload`. + // Nothing gates it now except the build's own capability, and this + // is the test that fails if a setting is ever wired back in. + assert!(should_download_in_background( + &supported(), + &sample_info("26.8.1") + )); + } + + #[test] + fn background_download_skips_builds_that_cannot_install() { + // Notify-only channels (Flatpak, Linux deb/rpm) can see the new + // version but never apply it, so downloading is pure waste. + let mut probe = probe("linux", Some("deb")); + probe.has_dpkg = true; + probe.os_release = Some("ID=ubuntu\n"); + let notify_only = support_from_probe(probe); + assert!(notify_only.can_check, "fixture should still be notify-only"); + + assert!(!should_download_in_background( + ¬ify_only, + &sample_info("26.8.1") + )); + // Belt and braces: the per-update flag says the same thing. + let not_installable = AppUpdateInfo { + installable: false, + ..sample_info("26.8.1") + }; + assert!(!should_download_in_background( + &supported(), + ¬_installable + )); + } + #[test] - fn take_downloaded_returns_bytes_for_matching_version() { - let runtime = AppUpdateRuntime::new(); - runtime.store_downloaded("26.8.1", vec![1, 2, 3]); + fn background_download_does_not_repeat_a_finished_download() { + let already_downloaded = AppUpdateInfo { + downloaded: true, + ..sample_info("26.8.1") + }; + assert!(!should_download_in_background( + &supported(), + &already_downloaded + )); + } + + #[test] + fn take_downloaded_returns_the_stored_package() { + let runtime = runtime(); + runtime.store_downloaded("26.8.1", (), vec![1, 2, 3]); assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.1")); - assert_eq!(runtime.take_downloaded("26.8.1"), Some(vec![1, 2, 3])); + + let package = runtime.take_downloaded().expect("package expected"); + assert_eq!(package.version, "26.8.1"); + assert_eq!(package.bytes, vec![1, 2, 3]); // Taking consumes the cache. assert_eq!(runtime.downloaded_version(), None); + assert!(runtime.take_downloaded().is_none()); } #[test] - fn take_downloaded_drops_stale_cache_on_version_mismatch() { - let runtime = AppUpdateRuntime::new(); - runtime.store_downloaded("26.8.1", vec![1, 2, 3]); - // A newer release appeared: the stale package must not be installed - // and must not linger in memory either. - assert_eq!(runtime.take_downloaded("26.8.2"), None); - assert_eq!(runtime.downloaded_version(), None); + fn store_downloaded_replaces_a_superseded_package() { + let runtime = runtime(); + runtime.store_downloaded("26.8.1", (), vec![1]); + // A newer release was found and downloaded: only one package is ever + // held, so the older one must not survive to be installed later. + runtime.store_downloaded("26.8.2", (), vec![2]); + + assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.2")); + let package = runtime.take_downloaded().expect("package expected"); + assert_eq!(package.version, "26.8.2"); + assert_eq!(package.bytes, vec![2]); } #[test] fn mark_downloaded_flags_recorded_check_and_returns_info() { - let runtime = AppUpdateRuntime::new(); + let runtime = runtime(); runtime.record_check(AppUpdateLastCheck { checked_at: "now".to_string(), update: Some(sample_info("26.8.1")), @@ -963,10 +1103,10 @@ mod tests { #[test] fn record_check_rederives_downloaded_from_byte_cache() { - let runtime = AppUpdateRuntime::new(); + let runtime = runtime(); // A background download finished between the check's cache read and // its recording: recording must not regress `downloaded` to false. - runtime.store_downloaded("26.8.1", vec![1]); + runtime.store_downloaded("26.8.1", (), vec![1]); let recorded = runtime.record_check(AppUpdateLastCheck { checked_at: "now".to_string(), update: Some(sample_info("26.8.1")), @@ -978,7 +1118,7 @@ mod tests { #[test] fn mark_downloaded_ignores_version_mismatch() { - let runtime = AppUpdateRuntime::new(); + let runtime = runtime(); runtime.record_check(AppUpdateLastCheck { checked_at: "now".to_string(), update: Some(sample_info("26.8.2")), diff --git a/src-tauri/src/config/types.rs b/src-tauri/src/config/types.rs index 387f6e8b..3a2450f1 100644 --- a/src-tauri/src/config/types.rs +++ b/src-tauri/src/config/types.rs @@ -760,21 +760,31 @@ mod tests { } #[test] - fn config_written_before_updates_became_mandatory_still_loads() { + fn config_written_before_updates_became_mandatory_keeps_its_settings() { // `autoUpdate.autoDownload` was a user setting until background - // downloads became mandatory on self-updating builds. Configs on - // disk still carry the key; loading must ignore it rather than - // failing and resetting the user's whole config to defaults. + // downloads became mandatory on self-updating builds. Configs on disk + // still carry the key, so the removed field must be ignored, NOT + // rejected: a parse error here would silently reset every real + // setting in the file to its default. Both asserted values are + // deliberately non-default so the test can fail. let legacy = r#"{ "version": 1, - "workspaceDirs": ["~/.clai/workspaces"], + "workspaceDirs": ["/tmp/clai-test-workspaces"], "mcpServers": [], "skillSources": [], "providerConnections": [], + "schedulerPaused": true, "autoUpdate": { "autoDownload": false } }"#; - let parsed: ClaiConfig = serde_json::from_str(legacy).unwrap(); - assert_eq!(parsed.workspace_dirs, default_workspace_dirs()); + let parsed: ClaiConfig = serde_json::from_str(legacy) + .expect("a config carrying the removed autoUpdate key must still load"); + + assert_eq!( + parsed.workspace_dirs, + vec![PathBuf::from("/tmp/clai-test-workspaces")], + ); + assert_ne!(parsed.workspace_dirs, default_workspace_dirs()); + assert!(parsed.scheduler_paused); } fn interval_kind(minutes: u32) -> crate::config::workspace_config::ScheduleKind { diff --git a/src/components/AppUpdateBadge.module.css b/src/components/AppUpdateBadge.module.css index 00efb964..b520c686 100644 --- a/src/components/AppUpdateBadge.module.css +++ b/src/components/AppUpdateBadge.module.css @@ -2,7 +2,10 @@ display: inline-flex; align-items: center; gap: 6px; - flex-shrink: 0; + min-width: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; padding: 4px 10px; border: 1px solid var(--color-primary); border-radius: 999px; @@ -31,7 +34,6 @@ display: inline-flex; align-items: center; gap: 8px; - flex-shrink: 0; min-width: 0; } @@ -60,14 +62,3 @@ opacity: 0.6; } -/* A failed install must not be silent — the click produced no restart, so say - why. Truncated to protect the top-bar layout; the full text is the title. */ -.error { - max-width: 220px; - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - color: var(--color-error); - font-size: 12px; - line-height: 1; -} diff --git a/src/components/AppUpdateBadge.test.tsx b/src/components/AppUpdateBadge.test.tsx index 66239021..4fc8f721 100644 --- a/src/components/AppUpdateBadge.test.tsx +++ b/src/components/AppUpdateBadge.test.tsx @@ -150,9 +150,13 @@ describe('AppUpdateBadge', () => { render(); await userEvent.click(await screen.findByRole('button', { name: /Restart to install/ })); + // The full reason goes to a live region (and the tooltip) rather than + // inline text, which would stretch the top bar by an arbitrary amount. expect(await screen.findByRole('alert')).toHaveTextContent('Permission denied'); // Re-enabled: a failed install must be retryable. - expect(screen.getByRole('button', { name: /Restart to install/ })).toBeEnabled(); + const retry = screen.getByRole('button', { name: /Install failed/ }); + expect(retry).toBeEnabled(); + expect(retry).toHaveAttribute('title', 'Permission denied'); }); it('clears a failed install when a newer version arrives', async () => { @@ -168,6 +172,7 @@ describe('AppUpdateBadge', () => { expect(await screen.findByText(/Update available · v26\.8\.2/)).toBeInTheDocument(); expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Restart to install/ })).toBeInTheDocument(); }); it('does not downgrade a downloaded update when a stale event arrives late', async () => { diff --git a/src/components/AppUpdateBadge.tsx b/src/components/AppUpdateBadge.tsx index 6e440fda..9e1038ec 100644 --- a/src/components/AppUpdateBadge.tsx +++ b/src/components/AppUpdateBadge.tsx @@ -12,9 +12,10 @@ * * Once the package has finished downloading in the background, a "Restart to * install" button appears beside the pill. That click is the ONLY thing that - * installs an update; the package is already on disk, so it is a restart, not - * a download wait. Ignoring it costs the user nothing — the app keeps running - * the current version and picks the download up again on the next launch. + * installs an update, and it needs no network: the backend applies the + * package it already downloaded and verified. Ignoring it costs the user + * nothing — the app keeps running the current version and picks the download + * up again on the next launch. */ import React, { useCallback, useState } from 'react'; @@ -43,9 +44,10 @@ const AppUpdateBadge = () => { setRestarting(true); setError(''); try { - // The backend installs the cached package and restarts the app, so a - // resolved promise is not the success path — the process is gone by - // then. Only the rejection matters here. + // Progress events are ignored on purpose: the package is already + // downloaded, so this is an extract-and-relaunch, not a transfer. The + // backend restarts the app on success, so a resolved promise is not + // the success path either — only the rejection matters here. await installAppUpdate(() => {}); setRestarting(false); } catch (err) { @@ -78,12 +80,16 @@ const AppUpdateBadge = () => { className={styles.action} onClick={restartToInstall} disabled={restarting} + // The reason lives in the tooltip and in the live region below: + // spelling it out inline would grow the top bar by an arbitrary + // amount of text and squeeze the workspace counters out. + title={error || undefined} > - {restarting ? 'Restarting...' : 'Restart to install'} + {restarting ? 'Restarting...' : error ? 'Install failed - retry' : 'Restart to install'} )} {error && ( - + {error} )} diff --git a/src/components/Settings/AboutSettings.test.tsx b/src/components/Settings/AboutSettings.test.tsx index f14ff140..d87a46cb 100644 --- a/src/components/Settings/AboutSettings.test.tsx +++ b/src/components/Settings/AboutSettings.test.tsx @@ -44,9 +44,9 @@ const notifyOnlyStatus = { lastCheck: { checkedAt: '2026-07-24T00:00:00Z', update: null, error: null }, }; -/** Phase 6 status: Linux deb/rpm with a published update that's still - * notify-only. The body copy and button label differ from the generic - * non-installable branch — this fixture exercises the platform branch. */ +/** Linux deb/rpm with a published update that is still notify-only. The body + * copy and button label differ from the generic non-installable branch — + * this fixture exercises the platform branch. */ const linuxPkgStatus = { support: { supported: false, diff --git a/src/components/Settings/AboutSettings.tsx b/src/components/Settings/AboutSettings.tsx index 1cb7ce64..55f308a5 100644 --- a/src/components/Settings/AboutSettings.tsx +++ b/src/components/Settings/AboutSettings.tsx @@ -124,7 +124,7 @@ const AboutSettings = () => { // summary line needs platform-aware copy: macOS/Windows users fall back // to the manual "Install and restart" / GitHub download, while Linux // builds are steward-managed by the user's package manager or Flatpak - // (notify-only by design; see Phase 6). + // (notify-only by design). const showPackageManagerHint = availableUpdate !== null && !availableUpdate.installable && isLinux; const updateSummary = installing diff --git a/src/layouts/FleetLayout.module.css b/src/layouts/FleetLayout.module.css index 2c05f02e..46fdb4de 100644 --- a/src/layouts/FleetLayout.module.css +++ b/src/layouts/FleetLayout.module.css @@ -23,8 +23,15 @@ display: block; } +/* The flexible middle of the bar: it absorbs the slack, and on a narrow + window it is what gives way. `min-width: 0` lets it actually shrink (a + flex item's default floor is its content), and clipping the counts is a + better outcome than pushing the update action or the settings button + off-screen. */ .counters { flex: 1; + min-width: 0; + overflow: hidden; display: flex; align-items: center; gap: 8px; From d93140691bc810d0e2e6f726adeb6ac3669dd4c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ca=C3=B1ete?= <2930882+juacker@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:21:28 +0200 Subject: [PATCH 3/3] fix(updates): never install a package the UI stopped offering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review found that dropping the version match from `take_downloaded` traded one silent failure for another. Reachable trace: 26.8.1 downloads and caches, a later check finds 26.8.2, its download fails, and Settings > About (whose install button does not wait for `downloaded`) then installs 26.8.1 while every surface says 26.8.2 — no error, no log line. `take_downloaded_if_current` restores the guard without giving up the offline fast path: the expected version comes from the last recorded check, which is already in memory, so the common case still touches no network. A mismatch drops the stale package and lets the install path fetch what is being offered. Also from review: - The pill's `text-overflow: ellipsis` was inert — it sat on a flex container, so a narrow window hard-clipped "v26.8.12" into the plausible-but-wrong "v26.8.". The label is now its own span that really ellipsizes, the dot no longer shrinks, and the pill (never the action button) is what gives way. - Corrected two comments that overstated the fast path: a click can still wait on `install_lock` behind an in-flight background download, which is reachable from About but not from the badge. --- src-tauri/src/commands/app_updates.rs | 86 +++++++++++++++++++----- src/components/AppUpdateBadge.module.css | 17 ++++- src/components/AppUpdateBadge.tsx | 2 +- src/layouts/FleetLayout.module.css | 11 +-- 4 files changed, 92 insertions(+), 24 deletions(-) diff --git a/src-tauri/src/commands/app_updates.rs b/src-tauri/src/commands/app_updates.rs index e41e8ba4..5c6f408f 100644 --- a/src-tauri/src/commands/app_updates.rs +++ b/src-tauri/src/commands/app_updates.rs @@ -30,9 +30,10 @@ const LATEST_MANIFEST_URL: &str = /// /// `installer` is the updater handle that produced `bytes`, kept alongside /// them. It is what makes "Restart to install" a restart rather than a -/// download: applying the package needs no network, so the click cannot be -/// derailed by an offline machine, by a slow manifest fetch, or by a newer -/// release appearing between the download and the click. +/// download: applying the package needs no network at all, so an offline +/// machine, a slow manifest fetch or an unreachable release host cannot stop +/// a click. A newer release appearing in the meantime does stop it — the +/// package is then stale; see `take_downloaded_if_current`. struct DownloadedPackage { version: String, installer: I, @@ -122,16 +123,24 @@ impl AppUpdateRuntime { }); } - /// Takes the downloaded package, whatever version it is. Installing - /// exactly what was downloaded is the point: it is the version the badge - /// offered to install, and it needs no network to apply. A newer release - /// is picked up by the next check, which replaces the cache. - fn take_downloaded(&self) -> Option> { + /// Takes the downloaded package, but only if it is the version the last + /// recorded check reports — the version every UI surface is offering. + /// + /// They can disagree: a later check can find a newer release whose own + /// download has not finished yet, or failed. Installing the older cached + /// package then would apply a version the user was never shown, so it is + /// dropped here and the install path fetches the offered one instead. + /// + /// Reads the two locks in sequence, never nested, so it cannot deadlock + /// against `record_check` (which reads them in the opposite order). + fn take_downloaded_if_current(&self) -> Option> { + let offered_version = self.last_check()?.update?.version; self.downloaded .lock() .expect("app update state poisoned") .package .take() + .filter(|package| package.version == offered_version) } /// Flags the recorded last check's update as downloaded (if it is still @@ -295,6 +304,10 @@ pub async fn install_app_update( state: State<'_, AppState>, on_event: Channel, ) -> Result<(), String> { + // Serialized against the background download: if one is in flight this + // waits for it instead of fetching the same package twice. The badge's + // action cannot land in that wait (it only appears once the download has + // finished), but Settings > About can, and shows its own progress line. let _install_guard = state.app_updates.install_lock.lock().await; let support = detect_support_status(); if !support.supported { @@ -309,7 +322,7 @@ pub async fn install_app_update( // cached with it. So there is nothing left to fetch — no manifest check, // no network at all — which is what lets the badge promise a restart // rather than a wait, offline included. - let package = match state.app_updates.take_downloaded() { + let package = match state.app_updates.take_downloaded_if_current() { Some(package) => { let _ = on_event.send(AppUpdateInstallEvent::DownloadFinished); package @@ -1060,29 +1073,72 @@ mod tests { } #[test] - fn take_downloaded_returns_the_stored_package() { + fn take_downloaded_if_current_returns_the_offered_package() { let runtime = runtime(); + runtime.record_check(AppUpdateLastCheck { + checked_at: "now".to_string(), + update: Some(sample_info("26.8.1")), + error: None, + }); runtime.store_downloaded("26.8.1", (), vec![1, 2, 3]); - assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.1")); - let package = runtime.take_downloaded().expect("package expected"); + assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.1")); + let package = runtime + .take_downloaded_if_current() + .expect("the offered version is cached"); assert_eq!(package.version, "26.8.1"); assert_eq!(package.bytes, vec![1, 2, 3]); - // Taking consumes the cache. + // Taking consumes the cache: one install, one package. assert_eq!(runtime.downloaded_version(), None); - assert!(runtime.take_downloaded().is_none()); + assert!(runtime.take_downloaded_if_current().is_none()); + } + + #[test] + fn take_downloaded_if_current_drops_a_superseded_package() { + let runtime = runtime(); + // 26.8.1 was downloaded, then a later check found 26.8.2 whose own + // download has not landed. Installing 26.8.1 now would apply a + // version no surface ever offered, so the stale package is dropped + // and the install path is left to fetch 26.8.2. + runtime.store_downloaded("26.8.1", (), vec![1]); + runtime.record_check(AppUpdateLastCheck { + checked_at: "now".to_string(), + update: Some(sample_info("26.8.2")), + error: None, + }); + + assert!(runtime.take_downloaded_if_current().is_none()); + assert_eq!(runtime.downloaded_version(), None, "must not linger"); + } + + #[test] + fn take_downloaded_if_current_keeps_the_package_when_no_check_is_recorded() { + // No check recorded means no version is being offered, so there is + // nothing to install: the package stays cached for a real offer. + let runtime = runtime(); + runtime.store_downloaded("26.8.1", (), vec![1]); + + assert!(runtime.take_downloaded_if_current().is_none()); + assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.1")); } #[test] fn store_downloaded_replaces_a_superseded_package() { let runtime = runtime(); + runtime.record_check(AppUpdateLastCheck { + checked_at: "now".to_string(), + update: Some(sample_info("26.8.2")), + error: None, + }); runtime.store_downloaded("26.8.1", (), vec![1]); // A newer release was found and downloaded: only one package is ever // held, so the older one must not survive to be installed later. runtime.store_downloaded("26.8.2", (), vec![2]); assert_eq!(runtime.downloaded_version().as_deref(), Some("26.8.2")); - let package = runtime.take_downloaded().expect("package expected"); + let package = runtime + .take_downloaded_if_current() + .expect("package expected"); assert_eq!(package.version, "26.8.2"); assert_eq!(package.bytes, vec![2]); } diff --git a/src/components/AppUpdateBadge.module.css b/src/components/AppUpdateBadge.module.css index b520c686..3ba0b14f 100644 --- a/src/components/AppUpdateBadge.module.css +++ b/src/components/AppUpdateBadge.module.css @@ -1,11 +1,12 @@ +/* Shrinkable (`min-width: 0`) so the pill, not the action button, is what + gives way when the top bar runs out of room. `text-overflow` clips a block + container's own inline content, and this is a flex container whose text + would become an anonymous flex item instead, hence the `.label` span. */ .badge { display: inline-flex; align-items: center; gap: 6px; min-width: 0; - overflow: hidden; - white-space: nowrap; - text-overflow: ellipsis; padding: 4px 10px; border: 1px solid var(--color-primary); border-radius: 999px; @@ -23,7 +24,17 @@ color: var(--color-bg-primary); } +/* Truncating a version string would show a plausible-looking wrong version + ("v26.8." for 26.8.12), so the ellipsis has to be real. */ +.label { + min-width: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; +} + .dot { + flex-shrink: 0; width: 7px; height: 7px; border-radius: 50%; diff --git a/src/components/AppUpdateBadge.tsx b/src/components/AppUpdateBadge.tsx index 9e1038ec..8c65d32e 100644 --- a/src/components/AppUpdateBadge.tsx +++ b/src/components/AppUpdateBadge.tsx @@ -72,7 +72,7 @@ const AppUpdateBadge = () => { aria-label={`Update available: CLAI v${update.version}`} >