diff --git a/INTERACTION_REVIEW.md b/INTERACTION_REVIEW.md new file mode 100644 index 0000000..5544966 --- /dev/null +++ b/INTERACTION_REVIEW.md @@ -0,0 +1,121 @@ +# Firelink interaction/functionality review + +Date: 2026-06-19 + +## Review method + +Code-traced visible UI actions through React components, Zustand stores, typed IPC wrappers, registered Tauri commands, Rust side effects, and frontend update/error paths. Build and Rust tests were run after fixes. + +## Root causes found + +1. The handwritten frontend IPC map had drifted from generated Rust bindings. + - Queue reordering sent `Up`/`Down`, but Rust deserializes `QueueDirection` as lowercase `up`/`down`. + +2. Some UI buttons were visually present but not wired. + - Add Download `Refresh Metadata` had no click handler. + +3. Some UI promises did not match the backend side effect. + - Settings global speed limit was labeled KiB/s, but bare numeric input was passed as bytes/s. + - Download Properties allowed editing speed for active transfers even though no backend command applied a per-download active speed change. + +4. Duplicate-resolution semantics were too broad. + - The duplicate dialog offered `Replace` for URL duplicates, but replacement only makes sense for destination-file conflicts. + +5. File-path actions used inconsistent destination resolution. + - `Copy File Path` bypassed the same effective destination logic used by Open/Reveal. + +6. Clipboard actions lacked visible error handling. + - Copy failures were previously silent. + +## Fixed + +### Main window + +| Surface/action | Status after review | Notes | +| --- | --- | --- | +| Start/resume paused or failed download | Works | Store enqueues through `enqueue_download`; existing aria2 GID resume path is handled by backend. | +| Pause active/queued/retrying download | Works | Frontend calls `pause_download`; backend removes pending task and pauses/removes active work as appropriate. | +| Remove download | Works | Frontend calls `remove_download`, then removes local persisted item. | +| Delete file with remove | Works | Uses `trash_download_assets` with backend ownership/path authorization. | +| Redownload | Works | Creates a new queued item with preserved metadata and enqueues through queue manager. | +| Open downloaded file | Works | Uses owned-path `open_downloaded_file`. | +| Show in Finder | Works for completed files/partials | Uses owned-path `reveal_in_file_manager`; non-completed double-click/menu behavior opens properties. | +| Copy URL/address | Fixed | Now reports clipboard errors. | +| Copy file path | Fixed | Now uses the same effective path resolution as file actions and reports clipboard errors. | +| Queue up/down ordering | Fixed | Sends lowercase `up`/`down` to match Rust-generated `QueueDirection`. | +| Double-click completed download | Works | Opens file; otherwise opens Properties. | +| Status/progress/speed/ETA updates | Works | Store listener handles progress/state events; existing media-size finalization behavior is preserved. | +| Column resize/layout | Works | Local UI-only state; no backend side effect. | + +### Add Download window + +| Surface/action | Status after review | Notes | +| --- | --- | --- | +| URL/multiple URL input | Works | Lines are parsed and invalid URLs surface per-item error state. | +| Paste/deep-link/extension prefill | Works | App-level paste/deep-link/extension handlers open the modal with URLs. | +| Metadata detection | Works | HTTP metadata and media metadata go through IPC. | +| Refresh Metadata | Fixed | Button now re-runs the existing parser/metadata flow. | +| Media format selection | Works | Selected format selector/ext is applied before enqueue. | +| File name/category/destination detection | Works | Category-specific destination routing is preserved. | +| Destination folder selection | Works | Uses Tauri directory picker. | +| Auth/site-login credentials | Works | Keychain-backed site-login password lookup is preserved. | +| Add paused / start queued | Works | Adds local item and enqueues when start is requested. | +| Duplicate URL conflict | Fixed | `Replace` is no longer offered for URL-only duplicates. | +| Duplicate file conflict | Works | Rename/replace/skip flows remain available for file conflicts. | +| Cancel/close | Works | Modal state is reset through store. | + +### Download Properties + +| Surface/action | Status after review | Notes | +| --- | --- | --- | +| Opens for queued/downloading/paused/failed/completed | Works | Selected item is read live from store. | +| Live progress/speed/ETA/size summary | Works | Summary uses current store item, not stale initial-only form state. | +| Save editable queued/paused/failed settings | Works | Updates local persisted item used by resume/redownload/enqueue paths. | +| Completed download settings for redownload | Works | Identity remains read-only; transfer settings can be saved for redownload. | +| Active-transfer speed controls | Fixed | Controls are locked while transfer is active; copy now states that current backend options remain active until pause/stop. | +| Browse destination | Works when not locked | Uses directory picker. | + +### Settings + +| Tab/action | Status after review | Notes | +| --- | --- | --- | +| Downloads tab settings | Fixed/Works | Bare global speed-limit values now normalize as KiB/s for live backend calls and newly queued downloads. | +| Look and feel tab | Works | Theme, font size, row density, dock badge, menu bar icon persist and drive app effects. | +| Network tab | Works | Proxy mode/host/port/user-agent values are used for enqueue payloads. | +| Locations tab | Works | Default/category paths persist; bulk category creation calls backend. | +| Site Logins tab | Works | Add/remove uses settings plus keychain password storage/deletion. | +| Power tab | Works | Prevent-sleep setting drives backend keep-awake effect. | +| Engine tab diagnostics | Works | Recheck calls registered engine status commands and surfaces errors/details. | +| Integrations tab | Works | Token copy/regenerate and extension pairing token updates are wired; token remains keychain-backed. | +| About tab updates | Works | Check Now calls update check and surfaces result via toast. | + +### Menus/context menus/dialog buttons + +| Surface/action | Status after review | Notes | +| --- | --- | --- | +| Download row context menu | Fixed/Works | Copy actions now resolve correctly and surface errors; start/pause/remove/redownload/properties remain wired. | +| Sidebar queue context menu | Works | Start/pause/rename/remove queues are wired through store. | +| Delete confirmation dialog | Works | Remove-only and remove+trash paths preserved. | +| Duplicate resolution dialog | Fixed | Replacement is now file-conflict-only. | + +### Backend IPC coverage + +| Check | Status | +| --- | --- | +| Frontend IPC command names exist in Rust registration | Works | +| Queue direction argument casing | Fixed | +| Global speed-limit unit handling | Fixed in frontend and backend startup/live command handling | +| File open/reveal/trash path authorization | Preserved | +| Generic duplicate-check/delete path guards | Preserved; no security loosening introduced | + +## Not fixed / follow-up + +- Full packaged GUI/manual click-through was not launched in this pass; validation was code-trace plus `npm run build` and Rust tests. +- Existing generic `check_file_exists` / `delete_file` remain scoped by `is_safe_path`, but they are broader than ownership-based open/reveal/trash commands. I did not loosen them. A future hardening pass should replace duplicate-file replacement with an explicit user-selected-destination trash command or ownership-aware pre-registration flow. +- Per-download speed changes for already-running transfers are not implemented because the current backend has no active-transfer per-item speed-limit IPC. The UI now stops promising that behavior. + +## Validation + +- `npm run build`: pass +- `cd src-tauri && cargo test --quiet`: pass + diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index a5a50c4..a90e8ec 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -2604,9 +2604,33 @@ async fn set_concurrent_limit(state: tauri::State<'_, AppState>, limit: usize) - Ok(()) } +fn normalize_speed_limit_for_aria2(limit: &str) -> Option { + let trimmed = limit.trim(); + if trimmed.is_empty() { + return None; + } + + let re = regex::Regex::new(r"(?i)^(\d+(?:\.\d+)?)\s*([kmgt]?)i?b?(?:/s)?$").ok()?; + let captures = re.captures(trimmed)?; + let amount = captures.get(1)?.as_str().parse::().ok()?; + if !amount.is_finite() || amount <= 0.0 { + return None; + } + + let unit = captures.get(2).map(|m| m.as_str().to_ascii_uppercase()).unwrap_or_default(); + Some(if unit.is_empty() { + format!("{amount}K") + } else { + format!("{amount}{unit}") + }) +} + #[tauri::command] async fn set_global_speed_limit(state: tauri::State<'_, AppState>, limit: Option) -> Result<(), String> { - let limit_str = limit.unwrap_or_else(|| "0".to_string()); + let limit_str = limit + .as_deref() + .and_then(normalize_speed_limit_for_aria2) + .unwrap_or_else(|| "0".to_string()); rpc_call( state.aria2_port, &state.aria2_secret, @@ -2850,12 +2874,21 @@ fn set_extension_frontend_ready( mod tests { use super::{ build_media_format_options, collect_download_uris, is_excluded_yt_dlp_format, json_lower, - media_progress_speed, parse_firelink_urls, parse_media_progress_line, MediaProgress, - MEDIA_PROGRESS_PREFIX, + media_progress_speed, normalize_speed_limit_for_aria2, parse_firelink_urls, + parse_media_progress_line, MediaProgress, MEDIA_PROGRESS_PREFIX, }; use serde_json::json; use std::time::{Duration, Instant}; + #[test] + fn normalizes_bare_global_speed_limits_as_kib_per_second() { + assert_eq!(normalize_speed_limit_for_aria2("1024"), Some("1024K".to_string())); + assert_eq!(normalize_speed_limit_for_aria2("512K"), Some("512K".to_string())); + assert_eq!(normalize_speed_limit_for_aria2("1.5 MB/s"), Some("1.5M".to_string())); + assert_eq!(normalize_speed_limit_for_aria2("0"), None); + assert_eq!(normalize_speed_limit_for_aria2("bad"), None); + } + #[test] fn collects_primary_url_and_unique_mirrors_in_order() { let uris = collect_download_uris( @@ -3257,9 +3290,9 @@ pub fn run() { .arg("--download-result=hide") .arg("--check-certificate=true"); - if !global_speed_limit.is_empty() { - cmd.arg(format!("--max-overall-download-limit={}", global_speed_limit)); - } + if let Some(global_speed_limit) = normalize_speed_limit_for_aria2(&global_speed_limit) { + cmd.arg(format!("--max-overall-download-limit={}", global_speed_limit)); + } cmd.stdout(std::process::Stdio::null()); cmd.stderr(std::process::Stdio::piped()); diff --git a/src/App.tsx b/src/App.tsx index d3f6bcb..389a3c3 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1,4 +1,4 @@ -import { initMediaDomains, isActiveDownloadStatus } from './utils/downloads'; +import { initMediaDomains, isActiveDownloadStatus, normalizeSpeedLimitForBackend } from './utils/downloads'; import { useEffect, useRef, useState } from "react"; import { Sidebar, SidebarFilter } from "./components/Sidebar"; import { DownloadTable } from "./components/DownloadTable"; @@ -102,19 +102,7 @@ function App() { if (previousSpeedLimit.current === globalSpeedLimit) return; previousSpeedLimit.current = globalSpeedLimit; - // Convert to aria2 format (e.g. "1M", "500K") - let formattedLimit = null; - if (globalSpeedLimit) { - const match = globalSpeedLimit.trim().match(/^(\d+(?:\.\d+)?)\s*([kmgt]?)b?(?:\/s)?$/i); - if (match) { - const amount = Number(match[1]); - if (Number.isFinite(amount) && amount > 0) { - const multipliers: Record = { '': 1, k: 1024, m: 1048576, g: 1073741824, t: 1099511627776 }; - const bytes = Math.round(amount * multipliers[match[2].toLowerCase()]); - formattedLimit = `${bytes}`; - } - } - } + const formattedLimit = normalizeSpeedLimitForBackend(globalSpeedLimit); invoke('set_global_speed_limit', { limit: formattedLimit }).catch(error => { console.error('Failed to apply global speed limit:', error); diff --git a/src/components/AddDownloadsModal.tsx b/src/components/AddDownloadsModal.tsx index 6551b40..a51b254 100644 --- a/src/components/AddDownloadsModal.tsx +++ b/src/components/AddDownloadsModal.tsx @@ -269,6 +269,7 @@ export const AddDownloadsModal = () => { const [selectedQueueId, setSelectedQueueId] = useState(MAIN_QUEUE_ID); const [urls, setUrls] = useState(''); + const [metadataRefreshNonce, setMetadataRefreshNonce] = useState(0); const [selectedItemIndex, setSelectedItemIndex] = useState(null); const [parsedItems, setParsedItems] = useState([]); @@ -464,7 +465,7 @@ export const AddDownloadsModal = () => { active = false; clearTimeout(timer); }; - }, [urls, pendingAddFilename, isSaveLocationManual]); + }, [urls, pendingAddFilename, isSaveLocationManual, metadataRefreshNonce]); if (!isAddModalOpen) return null; @@ -569,6 +570,7 @@ export const AddDownloadsModal = () => { const idx = parseInt(res.id); const item = itemsToAdd[idx]; if (!item) continue; + const conflict = conflicts.find(c => c.id === res.id); if (res.resolution === 'skip') { itemsToAdd[idx] = null; @@ -601,8 +603,12 @@ export const AddDownloadsModal = () => { } itemsToAdd[idx] = { ...item, file: newName }; - } else if (res.resolution === 'replace') { - let finalFile = item.file; + } else if (res.resolution === 'replace') { + if (conflict?.reason.type !== 'file') { + itemsToAdd[idx] = null; + continue; + } + let finalFile = item.file; if (item.isMedia && item.formats && item.selectedFormat !== undefined) { const selectedFormat = item.formats[item.selectedFormat]; const baseName = finalFile.substring(0, finalFile.lastIndexOf('.')) || finalFile; @@ -734,9 +740,13 @@ export const AddDownloadsModal = () => { />
{parsedItems.length} valid link(s) detected - +
diff --git a/src/components/DeleteConfirmationModal.tsx b/src/components/DeleteConfirmationModal.tsx index 8ec9b59..f7a7e6a 100644 --- a/src/components/DeleteConfirmationModal.tsx +++ b/src/components/DeleteConfirmationModal.tsx @@ -1,9 +1,11 @@ -import React from 'react'; +import React, { useState } from 'react'; import { useDownloadStore } from '../store/useDownloadStore'; import { AlertTriangle } from 'lucide-react'; export const DeleteConfirmationModal: React.FC = () => { const { deleteModalState, closeDeleteModal, removeDownload } = useDownloadStore(); + const [errorMessage, setErrorMessage] = useState(''); + const [isRemoving, setIsRemoving] = useState(false); if (!deleteModalState.isOpen) return null; @@ -11,58 +13,71 @@ export const DeleteConfirmationModal: React.FC = () => { closeDeleteModal(); }; - const handleRemoveFromList = () => { + const handleRemoveFromList = async () => { if (deleteModalState.downloadId) { - removeDownload(deleteModalState.downloadId, false); + setIsRemoving(true); + try { + await removeDownload(deleteModalState.downloadId, false); + } catch (error) { + setErrorMessage(`Remove failed: ${String(error)}`); + setIsRemoving(false); + return; + } } closeDeleteModal(); }; - const handleDeleteFile = () => { + const handleDeleteFile = async () => { if (deleteModalState.downloadId) { - removeDownload(deleteModalState.downloadId, true); + setIsRemoving(true); + try { + await removeDownload(deleteModalState.downloadId, true); + } catch (error) { + setErrorMessage(`Delete failed: ${String(error)}`); + setIsRemoving(false); + return; + } } closeDeleteModal(); }; return (
-
e.stopPropagation()} > - {/* Header */}
-

- Remove Download -

+

Remove Download

- {/* Body */}
Are you sure you want to remove this item from the list? You can also choose to delete the underlying file from your hard drive. + {errorMessage &&
{errorMessage}
}
- {/* Footer */}
diff --git a/src/components/DownloadItem.tsx b/src/components/DownloadItem.tsx index 430dd18..62c4030 100644 --- a/src/components/DownloadItem.tsx +++ b/src/components/DownloadItem.tsx @@ -178,7 +178,7 @@ export const DownloadItem = React.memo(({ {download.status === 'queued' && queueIndex !== -1 && ( <> @@ -404,8 +406,16 @@ export const DownloadTable: React.FC = ({ filter }) => {
diff --git a/src/components/PropertiesModal.tsx b/src/components/PropertiesModal.tsx index 018f811..03e1e17 100644 --- a/src/components/PropertiesModal.tsx +++ b/src/components/PropertiesModal.tsx @@ -190,7 +190,7 @@ export const PropertiesModal = () => { {item.status === 'completed' ? "File identity is read-only. Transfer settings are saved for redownload." - : "Only the speed limit applies to the current transfer. Other settings can be changed after stopping or pausing."} + : "Transfer settings can be changed after stopping or pausing. Current transfers keep their existing backend options."}
)} @@ -222,12 +222,12 @@ export const PropertiesModal = () => {
{speedLimitEnabled && (
- setSpeedLimitValue(e.target.value)} className="w-20 bg-bg-input border border-border-modal rounded-lg px-2.5 py-1.5 text-xs text-text-primary focus:outline-none focus:border-accent" /> + setSpeedLimitValue(e.target.value)} disabled={isTransferLocked} className="w-20 bg-bg-input border border-border-modal rounded-lg px-2.5 py-1.5 text-xs text-text-primary focus:outline-none focus:border-accent disabled:opacity-50" /> KiB/s
)} diff --git a/src/ipc.ts b/src/ipc.ts index ab2eff2..c25471b 100644 --- a/src/ipc.ts +++ b/src/ipc.ts @@ -109,7 +109,7 @@ type CommandMap = { get_pending_order: { args: undefined; result: string[] }; enqueue_download: { args: { item: any }; result: string }; enqueue_many: { args: { items: any[] }; result: void }; - move_in_queue: { args: { id: string; direction: 'Up' | 'Down' | 'Top' | 'Bottom' }; result: string[] }; + move_in_queue: { args: { id: string; direction: 'up' | 'down' }; result: string[] }; remove_from_queue: { args: { id: string }; result: boolean }; }; diff --git a/src/store/useDownloadStore.ts b/src/store/useDownloadStore.ts index 1b171ed..c7c3772 100644 --- a/src/store/useDownloadStore.ts +++ b/src/store/useDownloadStore.ts @@ -11,7 +11,7 @@ import type { ExtensionDownload } from '../bindings/ExtensionDownload'; import type { Queue } from '../bindings/Queue'; import type { MediaMetadata } from '../bindings/MediaMetadata'; import { useSettingsStore } from './useSettingsStore'; -import { isActiveDownloadStatus, redactDownloadForPersistence } from '../utils/downloads'; +import { isActiveDownloadStatus, normalizeSpeedLimitForBackend, redactDownloadForPersistence } from '../utils/downloads'; import { fetchMediaMetadataDeduped } from '../utils/mediaMetadata'; export type { DownloadCategory } from '../utils/downloads'; @@ -98,7 +98,7 @@ interface DownloadState { queues: Queue[]; pendingOrder: string[]; setPendingOrder: (order: string[]) => void; - moveInQueue: (id: string, direction: 'Up' | 'Down') => Promise; + moveInQueue: (id: string, direction: 'up' | 'down') => Promise; removeFromQueue: (id: string) => Promise; isAddModalOpen: boolean; pendingAddUrls: string; @@ -235,7 +235,7 @@ export const useDownloadStore = create((set, get) => ({ destination: destPath, filename: item.fileName, connections: item.connections || settings.perServerConnections || null, - speed_limit: item.speedLimit || settings.globalSpeedLimit || null, + speed_limit: item.speedLimit || normalizeSpeedLimitForBackend(settings.globalSpeedLimit), username: item.username || (login ? login.username : null), password: item.password || keychainPassword, headers: item.headers || null, @@ -283,24 +283,22 @@ export const useDownloadStore = create((set, get) => ({ }, removeDownload: async (id, deleteFile = false) => { const item = get().downloads.find(d => d.id === id); + + if (item && deleteFile) { + const filepath = await resolveDownloadPath(item.destination || '~/Downloads', item.fileName); + const partialPaths = [`${filepath}.aria2`, `${filepath}.part`]; + await invoke('trash_download_assets', { path: filepath, partialPaths }); + } + if (item) { try { await invoke('remove_download', { id, filepath: null }); } catch (e) { console.error("Failed to terminate download on deletion:", e); - return; - } - } - - if (item && deleteFile) { - try { - const filepath = await resolveDownloadPath(item.destination || '~/Downloads', item.fileName); - const partialPaths = [`${filepath}.aria2`, `${filepath}.part`]; - await invoke('trash_download_assets', { path: filepath, partialPaths }); - } catch (e) { - console.error("Failed to trash file from disk:", e); + throw e; } } + set((state) => ({ downloads: state.downloads.filter(d => d.id !== id), pendingOrder: state.pendingOrder.filter(x => x !== id) @@ -385,7 +383,7 @@ export const useDownloadStore = create((set, get) => ({ destination: destPath, filename, connections: targetItem.connections || settings.perServerConnections || null, - speed_limit: targetItem.speedLimit || settings.globalSpeedLimit || null, + speed_limit: targetItem.speedLimit || normalizeSpeedLimitForBackend(settings.globalSpeedLimit), username: targetItem.username || (login ? login.username : null), password: targetItem.password || keychainPassword, headers: targetItem.headers || null, @@ -448,7 +446,7 @@ export const useDownloadStore = create((set, get) => ({ destination: destPath, filename: targetItem.fileName, connections: targetItem.connections || settings.perServerConnections || null, - speed_limit: targetItem.speedLimit || settings.globalSpeedLimit || null, + speed_limit: targetItem.speedLimit || normalizeSpeedLimitForBackend(settings.globalSpeedLimit), username: targetItem.username || (login ? login.username : null), password: targetItem.password || keychainPassword, headers: targetItem.headers || null, @@ -512,7 +510,7 @@ export const useDownloadStore = create((set, get) => ({ destination: destPath, filename: item.fileName, connections: item.connections || settings.perServerConnections || null, - speed_limit: item.speedLimit || settings.globalSpeedLimit || null, + speed_limit: item.speedLimit || normalizeSpeedLimitForBackend(settings.globalSpeedLimit), username: item.username || (login ? login.username : null), password: item.password || keychainPassword, headers: item.headers || null, @@ -631,7 +629,7 @@ export const useDownloadStore = create((set, get) => ({ destination: destPath, filename: item.fileName, connections: item.connections || settings.perServerConnections || null, - speed_limit: item.speedLimit || settings.globalSpeedLimit || null, + speed_limit: item.speedLimit || normalizeSpeedLimitForBackend(settings.globalSpeedLimit), username: item.username || (login ? login.username : null), password: item.password || keychainPassword, headers: item.headers || null, diff --git a/src/utils/downloads.ts b/src/utils/downloads.ts index 0d4ae24..5fc995a 100644 --- a/src/utils/downloads.ts +++ b/src/utils/downloads.ts @@ -31,6 +31,20 @@ const ACTIVE_DOWNLOAD_STATUSES: ReadonlySet = new Set([ export const isActiveDownloadStatus = (status: DownloadStatus): boolean => ACTIVE_DOWNLOAD_STATUSES.has(status); +export const normalizeSpeedLimitForBackend = (value?: string | null): string | null => { + const trimmed = value?.trim(); + if (!trimmed) return null; + + const match = trimmed.match(/^(\d+(?:\.\d+)?)\s*([kmgt]?)i?b?(?:\/s)?$/i); + if (!match) return null; + + const amount = Number(match[1]); + if (!Number.isFinite(amount) || amount <= 0) return null; + + const unit = match[2].toUpperCase(); + return unit ? `${amount}${unit}` : `${amount}K`; +}; + export const initMediaDomains = async () => { try { const domains = await invoke('get_supported_media_domains');