Skip to content

Commit 71de96e

Browse files
committed
IPC hardening: actually make the UX nice
Backend: - Add TimedOut<T> and IpcError types so frontend can distinguish timeouts from real empty/none results - Add force-timeouts Cargo feature for QA (FORCE_TIMEOUTS=1 pnpm dev) Frontend — transparent timeout UI: - Volume dropdown: "Some volumes may be missing" warning row + retry - Volume space bars: dashed placeholder with spinner/shake retry cycle, auto-retry after 5s, debounced clicks, context-aware tooltips - Tab restore: inline banner with "Retry" / "Open home folder" instead of silently falling back to default volume; warning icon on tab - Write ops (create/rename/trash): honest "may have succeeded" message with "Refresh listing" action instead of generic error - File viewer: retry+cancel buttons for timeout errors, visually distinct from permanent errors - Silent auto-retry for cosmetic data (sync badges, icons) after 5s Bug fix: - Dir-exists poll now requires 2 consecutive "not exists" before navigating away, preventing false navigation on slow mounts
1 parent 6a58278 commit 71de96e

43 files changed

Lines changed: 1400 additions & 297 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/desktop/coverage-allowlist.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@
5353
"file-explorer/navigation/path-navigation.ts": { "reason": "Depends on Tauri pathExists command" },
5454
"file-explorer/network/NetworkLoginForm.svelte": { "reason": "Network component, needs Tauri integration" },
5555
"file-explorer/pane/PermissionDeniedPane.svelte": { "reason": "Simple UI component" },
56+
"file-explorer/pane/VolumeUnreachableBanner.svelte": {
57+
"reason": "UI banner component, depends on Tauri commands"
58+
},
5659
"file-explorer/selection/SelectionInfo.svelte": {
5760
"reason": "Logic tested in selection-info-utils.ts, DOM-dependent"
5861
},
@@ -118,6 +121,7 @@
118121
"tauri-commands/mtp.ts": { "reason": "Tauri command wrappers, tested via integration" },
119122
"tauri-commands/networking.ts": { "reason": "Tauri command wrappers, tested via integration" },
120123
"tauri-commands/settings.ts": { "reason": "Tauri command wrappers, tested via integration" },
124+
"tauri-commands/ipc-types.ts": { "reason": "Type definitions and small helpers, tested via consuming modules" },
121125
"tauri-commands/storage.ts": { "reason": "Tauri command wrappers, tested via integration" },
122126
"tauri-commands/write-operations.ts": { "reason": "Tauri command wrappers, tested via integration" },
123127
"utils/confirm-dialog.ts": { "reason": "Thin wrapper over Tauri dialog API" },

apps/desktop/src-tauri/src/commands/CLAUDE.md

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ immediately to business-logic modules. No significant logic lives here.
88
| File | Domain | Notes |
99
|------|--------|-------|
1010
| `mod.rs` | Re-exports | `mtp`, `network` gated behind `#[cfg(any(target_os = "macos", target_os = "linux"))]`; `volumes` behind `#[cfg(target_os = "macos")]`; `volumes_linux` behind `#[cfg(target_os = "linux")]` |
11-
| `util.rs` | Shared helpers | `blocking_with_timeout` — runs a blocking closure on the blocking thread pool with a timeout, returning a fallback value on timeout. Used by all filesystem-touching commands. |
11+
| `util.rs` | Shared helpers | `TimedOut<T>`, `IpcError`, `blocking_with_timeout`, `blocking_with_timeout_flag`, `blocking_result_with_timeout`. See "Timeout-aware return types" below. |
1212
| `file_system.rs` | File listing & writes | Largest file. Streaming + virtual-scroll listing API, write ops (copy, move, delete, trash), scan preview, conflict resolution, volume copy, native drag, self-drag overlay. Contains `expand_tilde()`. |
1313
| `volumes.rs` | Volume management (macOS) | `list_volumes`, `get_default_volume_id`, `find_containing_volume`, `get_volume_space` |
1414
| `volumes_linux.rs` | Volume management (Linux) | Same interface as `volumes.rs`, delegates to `volumes_linux` module |
@@ -35,14 +35,24 @@ immediately to business-logic modules. No significant logic lives here.
3535
**Decision**: `blocking_with_timeout` for all filesystem-touching commands, not just read-only ones.
3636
**Why**: `spawn_blocking` alone doesn't protect against hung NFS/SMB mounts where even a simple `path.exists()` can block indefinitely. The timeout wrapper (2s for reads, 5s for writes, 15s for trash, 30s for recursive scans) returns a fallback value (or error for `Result`-returning commands) instead of freezing the IPC thread or exhausting the blocking pool. The helper lives in `util.rs` so all command files can share it. Commands that already use `spawn_blocking` (P2 commands like `rename_file`, `move_to_trash`) wrap the existing `spawn_blocking` with `tokio::time::timeout` instead.
3737

38+
**Decision**: Timeout-aware return types (`TimedOut<T>` and `IpcError`) for all timeout-protected commands.
39+
**Why**: The original `blocking_with_timeout` returned a fallback value indistinguishable from a real empty/none result. The frontend couldn't tell "no volumes mounted" from "timed out before listing volumes." Two wrapper types solve this:
40+
- `TimedOut<T>` (`{ data: T, timedOut: bool }`) — for commands that don't return `Result` (collections, `Option`, `()`). Use `blocking_with_timeout_flag` to produce these.
41+
- `IpcError` (`{ message: String, timedOut: bool }`) — for commands returning `Result<T, _>`. Use `blocking_result_with_timeout` or map `tokio::time::timeout` errors to `IpcError::timeout()`.
42+
The frontend has matching TypeScript types in `$lib/tauri-commands/ipc-types.ts` (`TimedOut<T>`, `IpcError`, `isIpcError`, `getIpcErrorMessage`). The old `blocking_with_timeout` is kept for `path_exists` and other commands where timeout distinction isn't needed.
43+
3844
**Decision**: No `commands/ai.rs` file -- AI commands register directly from `ai::manager` and `ai::suggestions`.
3945
**Why**: The AI subsystem has its own complex lifecycle (model loading, suggestion pipelines). Adding a thin wrapper in `commands/` would just be boilerplate forwarding. Registering directly keeps the AI command surface co-located with its implementation, which changes frequently.
4046

4147
## Key patterns and gotchas
4248

4349
- **No business logic here.** If you find yourself adding branching or data transformation, move it to the relevant subsystem module.
4450
- **`spawn_blocking` for filesystem I/O.** All blocking operations in async commands are wrapped in `tokio::task::spawn_blocking`.
45-
- **`blocking_with_timeout` for potentially slow I/O.** All filesystem-touching commands use either `blocking_with_timeout` (from `util.rs`) or `tokio::time::timeout` around `spawn_blocking`. Timeouts: 2s for reads, 5s for writes (`create_directory`, `rename_file`), 15s for trash (`move_to_trash` — macOS NSFileManager is slow on cold start), 30s for recursive scans (`scan_volume_for_copy`, `scan_volume_for_conflicts`). The helper returns a fallback value on timeout or `JoinError`.
51+
- **Timeout-protected I/O with distinguishable results.** All filesystem-touching commands use timeout wrappers from `util.rs`. Timeouts: 2s for reads, 5s for writes (`create_directory`, `rename_file`), 15s for trash (`move_to_trash`), 30s for recursive scans. Three helpers:
52+
- `blocking_with_timeout(duration, fallback, closure)` — returns raw `T`, timeout indistinguishable from fallback. Used only where distinction isn't needed (like `path_exists`).
53+
- `blocking_with_timeout_flag(duration, fallback, closure)``TimedOut<T>` — for commands returning `Vec`, `HashMap`, `Option`, or `()`. Frontend unwraps `.data` and can check `.timedOut`.
54+
- `blocking_result_with_timeout(duration, closure)``Result<T, IpcError>` — for commands that already return `Result`. Timeout becomes `Err(IpcError { message, timedOut: true })`.
55+
- For P2 commands using raw `tokio::time::timeout`, map the `Elapsed` error to `IpcError::timeout()` and other errors to `IpcError::from_err(msg)`.
4656
- **`expand_tilde`** is applied conditionally: for `list_directory` it's gated on `volume_id == "root"`, but for write operations (copy, move, delete, scan preview) it's always applied. MTP and network volume paths must never be tilde-expanded.
4757
- **AI commands** are registered directly from `ai::manager` and `ai::suggestions` — there is no `commands/ai.rs` file.
4858
- **Platform gates.** `volumes` is macOS-only; `mtp` and `network` are macOS+Linux; `volumes_linux` is Linux-only. Individual functions also use `#[cfg]` where behaviour differs (e.g., `sync_status`).

apps/desktop/src-tauri/src/commands/file_system.rs

Lines changed: 48 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ use std::sync::mpsc::channel;
2929
use tauri::Manager;
3030
use tokio::time::Duration;
3131

32-
use super::util::blocking_with_timeout;
32+
use super::util::{
33+
IpcError, TimedOut, blocking_result_with_timeout, blocking_with_timeout, blocking_with_timeout_flag,
34+
};
3335

3436
const PATH_EXISTS_TIMEOUT: Duration = Duration::from_secs(2);
3537

@@ -55,12 +57,16 @@ pub async fn path_exists(volume_id: Option<String>, path: String) -> bool {
5557
}
5658

5759
#[tauri::command]
58-
pub async fn create_directory(volume_id: Option<String>, parent_path: String, name: String) -> Result<String, String> {
60+
pub async fn create_directory(
61+
volume_id: Option<String>,
62+
parent_path: String,
63+
name: String,
64+
) -> Result<String, IpcError> {
5965
if name.is_empty() {
60-
return Err("Folder name cannot be empty".to_string());
66+
return Err(IpcError::from_err("Folder name cannot be empty"));
6167
}
6268
if name.contains('/') || name.contains('\0') {
63-
return Err("Folder name contains invalid characters".to_string());
69+
return Err(IpcError::from_err("Folder name contains invalid characters"));
6470
}
6571

6672
let volume_id = volume_id.unwrap_or_else(|| "root".to_string());
@@ -85,7 +91,9 @@ pub async fn create_directory(volume_id: Option<String>, parent_path: String, na
8591
Duration::from_secs(5),
8692
tokio::task::spawn_blocking(move || {
8793
volume.create_directory(&new_path_clone).map_err(|e| match e {
88-
crate::file_system::VolumeError::AlreadyExists(_) => format!("'{}' already exists", name_clone),
94+
crate::file_system::VolumeError::AlreadyExists(_) => {
95+
format!("'{}' already exists", name_clone)
96+
}
8997
crate::file_system::VolumeError::PermissionDenied(_) => {
9098
format!(
9199
"Permission denied: cannot create '{}' in '{}'",
@@ -97,22 +105,25 @@ pub async fn create_directory(volume_id: Option<String>, parent_path: String, na
97105
}),
98106
)
99107
.await
100-
.map_err(|_| "Operation timed out (network mount may be unresponsive)".to_string())?
101-
.map_err(|e| format!("Task failed: {}", e))??;
108+
.map_err(|_| IpcError::timeout())?
109+
.map_err(|e| IpcError::from_err(format!("Task failed: {}", e)))?
110+
.map_err(IpcError::from_err)?;
102111

103112
return Ok(new_path.to_string_lossy().to_string());
104113
}
105114

106115
// Fallback for unknown volumes (shouldn't happen in practice)
107116
let mut new_path = PathBuf::from(&expanded_path);
108117
new_path.push(&name);
109-
std::fs::create_dir(&new_path).map_err(|e| match e.kind() {
110-
std::io::ErrorKind::AlreadyExists => format!("'{}' already exists", name),
111-
std::io::ErrorKind::PermissionDenied => {
112-
format!("Permission denied: cannot create '{}' in '{}'", name, parent_path)
113-
}
114-
_ => format!("Failed to create folder: {}", e),
115-
})?;
118+
std::fs::create_dir(&new_path)
119+
.map_err(|e| match e.kind() {
120+
std::io::ErrorKind::AlreadyExists => format!("'{}' already exists", name),
121+
std::io::ErrorKind::PermissionDenied => {
122+
format!("Permission denied: cannot create '{}' in '{}'", name, parent_path)
123+
}
124+
_ => format!("Failed to create folder: {}", e),
125+
})
126+
.map_err(IpcError::from_err)?;
116127
Ok(new_path.to_string_lossy().to_string())
117128
}
118129

@@ -128,18 +139,14 @@ pub async fn list_directory_start(
128139
sort_by: SortColumn,
129140
sort_order: SortOrder,
130141
directory_sort_mode: Option<DirectorySortMode>,
131-
) -> Result<ListingStartResult, String> {
142+
) -> Result<ListingStartResult, IpcError> {
132143
let expanded_path = expand_tilde(&path);
133144
let path_buf = PathBuf::from(&expanded_path);
134145
let dir_sort_mode = directory_sort_mode.unwrap_or_default();
135-
blocking_with_timeout(
136-
Duration::from_secs(2),
137-
Err("Operation timed out (network mount may be unresponsive)".to_string()),
138-
move || {
139-
ops_list_directory_start_with_volume("root", &path_buf, include_hidden, sort_by, sort_order, dir_sort_mode)
140-
.map_err(|e| format!("Failed to start directory listing '{}': {}", path, e))
141-
},
142-
)
146+
blocking_result_with_timeout(Duration::from_secs(2), move || {
147+
ops_list_directory_start_with_volume("root", &path_buf, include_hidden, sort_by, sort_order, dir_sort_mode)
148+
.map_err(|e| format!("Failed to start directory listing '{}': {}", path, e))
149+
})
143150
.await
144151
}
145152

@@ -247,11 +254,11 @@ pub fn list_directory_end(listing_id: String) {
247254
/// Force a re-read of a watched directory listing, emitting any diff.
248255
/// Used after write operations (move) when the file watcher may not fire promptly.
249256
#[tauri::command]
250-
pub async fn refresh_listing(listing_id: String) {
251-
blocking_with_timeout(Duration::from_secs(2), (), move || {
257+
pub async fn refresh_listing(listing_id: String) -> TimedOut<()> {
258+
blocking_with_timeout_flag(Duration::from_secs(2), (), move || {
252259
crate::file_system::watcher::handle_directory_change(&listing_id);
253260
})
254-
.await;
261+
.await
255262
}
256263

257264
/// Returns total file/dir counts and sizes, plus selection stats if `selected_indices` is given.
@@ -524,14 +531,14 @@ pub async fn scan_volume_for_copy(
524531
dest_volume_id: String,
525532
dest_path: String,
526533
max_conflicts: Option<usize>,
527-
) -> Result<VolumeCopyScanResult, String> {
534+
) -> Result<VolumeCopyScanResult, IpcError> {
528535
let source_volume = get_volume_manager()
529536
.get(&source_volume_id)
530-
.ok_or_else(|| format!("Source volume '{}' not found", source_volume_id))?;
537+
.ok_or_else(|| IpcError::from_err(format!("Source volume '{}' not found", source_volume_id)))?;
531538

532539
let dest_volume = get_volume_manager()
533540
.get(&dest_volume_id)
534-
.ok_or_else(|| format!("Destination volume '{}' not found", dest_volume_id))?;
541+
.ok_or_else(|| IpcError::from_err(format!("Destination volume '{}' not found", dest_volume_id)))?;
535542

536543
let source_paths: Vec<PathBuf> = source_paths.iter().map(PathBuf::from).collect();
537544
let dest_path = PathBuf::from(dest_path);
@@ -546,8 +553,9 @@ pub async fn scan_volume_for_copy(
546553
}),
547554
)
548555
.await
549-
.map_err(|_| "Operation timed out (network mount may be unresponsive)".to_string())?
550-
.map_err(|e| format!("Scan task failed: {}", e))?
556+
.map_err(|_| IpcError::timeout())?
557+
.map_err(|e| IpcError::from_err(format!("Scan task failed: {}", e)))?
558+
.map_err(IpcError::from_err)
551559
}
552560

553561
/// Checks which source items already exist at the destination. Returns conflict details for UI.
@@ -556,10 +564,10 @@ pub async fn scan_volume_for_conflicts(
556564
volume_id: String,
557565
source_items: Vec<SourceItemInput>,
558566
dest_path: String,
559-
) -> Result<Vec<ScanConflict>, String> {
567+
) -> Result<Vec<ScanConflict>, IpcError> {
560568
let volume = get_volume_manager()
561569
.get(&volume_id)
562-
.ok_or_else(|| format!("Volume '{}' not found", volume_id))?;
570+
.ok_or_else(|| IpcError::from_err(format!("Volume '{}' not found", volume_id)))?;
563571

564572
let source_items: Vec<crate::file_system::SourceItemInfo> = source_items
565573
.into_iter()
@@ -581,8 +589,9 @@ pub async fn scan_volume_for_conflicts(
581589
}),
582590
)
583591
.await
584-
.map_err(|_| "Operation timed out (network mount may be unresponsive)".to_string())?
585-
.map_err(|e| format!("Conflict scan task failed: {}", e))?
592+
.map_err(|_| IpcError::timeout())?
593+
.map_err(|e| IpcError::from_err(format!("Conflict scan task failed: {}", e)))?
594+
.map_err(IpcError::from_err)
586595
}
587596

588597
/// Input type for source item information (used by scan_volume_for_conflicts).
@@ -693,7 +702,7 @@ mod tests {
693702
fs::create_dir(tmp.join("existing")).unwrap();
694703
let result = create_directory(None, parent, "existing".to_string()).await;
695704
assert!(result.is_err());
696-
assert!(result.unwrap_err().contains("already exists"));
705+
assert!(result.unwrap_err().message.contains("already exists"));
697706
cleanup_test_dir(&tmp);
698707
}
699708

@@ -703,7 +712,7 @@ mod tests {
703712
let parent = tmp.to_string_lossy().to_string();
704713
let result = create_directory(None, parent, "".to_string()).await;
705714
assert!(result.is_err());
706-
assert!(result.unwrap_err().contains("cannot be empty"));
715+
assert!(result.unwrap_err().message.contains("cannot be empty"));
707716
cleanup_test_dir(&tmp);
708717
}
709718

@@ -713,11 +722,11 @@ mod tests {
713722
let parent = tmp.to_string_lossy().to_string();
714723
let result = create_directory(None, parent.clone(), "foo/bar".to_string()).await;
715724
assert!(result.is_err());
716-
assert!(result.unwrap_err().contains("invalid characters"));
725+
assert!(result.unwrap_err().message.contains("invalid characters"));
717726

718727
let result = create_directory(None, parent, "foo\0bar".to_string()).await;
719728
assert!(result.is_err());
720-
assert!(result.unwrap_err().contains("invalid characters"));
729+
assert!(result.unwrap_err().message.contains("invalid characters"));
721730
cleanup_test_dir(&tmp);
722731
}
723732

apps/desktop/src-tauri/src/commands/file_viewer.rs

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
33
use tokio::time::Duration;
44

5-
use super::util::blocking_with_timeout;
5+
use super::util::{IpcError, blocking_result_with_timeout};
66
use crate::file_viewer::{self, LineChunk, SearchPollResult, SeekTarget, ViewerOpenResult, ViewerSessionStatus};
77
use log::debug;
88
use tauri::Manager;
@@ -13,12 +13,10 @@ const VIEWER_TIMEOUT: Duration = Duration::from_secs(2);
1313
/// Opens a viewer session for the given file.
1414
/// Returns session metadata + initial lines from the start of the file.
1515
#[tauri::command]
16-
pub async fn viewer_open(path: String) -> Result<ViewerOpenResult, String> {
17-
blocking_with_timeout(
18-
VIEWER_TIMEOUT,
19-
Err("Operation timed out (network mount may be unresponsive)".to_string()),
20-
move || file_viewer::open_session(&path).map_err(|e| e.to_string()),
21-
)
16+
pub async fn viewer_open(path: String) -> Result<ViewerOpenResult, IpcError> {
17+
blocking_result_with_timeout(VIEWER_TIMEOUT, move || {
18+
file_viewer::open_session(&path).map_err(|e| e.to_string())
19+
})
2220
.await
2321
}
2422

@@ -35,16 +33,16 @@ pub async fn viewer_get_lines(
3533
target_type: String,
3634
target_value: f64,
3735
count: usize,
38-
) -> Result<LineChunk, String> {
36+
) -> Result<LineChunk, IpcError> {
3937
let target = match target_type.as_str() {
4038
"line" => SeekTarget::Line(target_value as usize),
4139
"byte" => SeekTarget::ByteOffset(target_value as u64),
4240
"fraction" => SeekTarget::Fraction(target_value),
4341
other => {
44-
return Err(format!(
42+
return Err(IpcError::from_err(format!(
4543
"Unknown target type: {}. Use 'line', 'byte', or 'fraction'.",
4644
other
47-
));
45+
)));
4846
}
4947
};
5048

@@ -53,11 +51,9 @@ pub async fn viewer_get_lines(
5351
session_id, target_type, target_value, count
5452
);
5553

56-
let result = blocking_with_timeout(
57-
VIEWER_TIMEOUT,
58-
Err("Operation timed out (network mount may be unresponsive)".to_string()),
59-
move || file_viewer::get_lines(&session_id, target, count).map_err(|e| e.to_string()),
60-
)
54+
let result = blocking_result_with_timeout(VIEWER_TIMEOUT, move || {
55+
file_viewer::get_lines(&session_id, target, count).map_err(|e| e.to_string())
56+
})
6157
.await?;
6258

6359
debug!(

apps/desktop/src-tauri/src/commands/icons.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
use std::collections::HashMap;
44
use tokio::time::Duration;
55

6-
use super::util::blocking_with_timeout;
6+
use super::util::{TimedOut, blocking_with_timeout_flag};
77
use crate::icons;
88

99
const ICONS_TIMEOUT: Duration = Duration::from_secs(2);
@@ -16,8 +16,8 @@ const ICONS_TIMEOUT: Duration = Duration::from_secs(2);
1616
/// are fetched from app bundles (showing the app's icon as fallback). When false,
1717
/// the system's default document icons are used (Finder-style with app badge).
1818
#[tauri::command]
19-
pub async fn get_icons(icon_ids: Vec<String>, use_app_icons_for_documents: bool) -> HashMap<String, String> {
20-
blocking_with_timeout(ICONS_TIMEOUT, HashMap::new(), move || {
19+
pub async fn get_icons(icon_ids: Vec<String>, use_app_icons_for_documents: bool) -> TimedOut<HashMap<String, String>> {
20+
blocking_with_timeout_flag(ICONS_TIMEOUT, HashMap::new(), move || {
2121
icons::get_icons(icon_ids, use_app_icons_for_documents)
2222
})
2323
.await
@@ -34,8 +34,8 @@ pub async fn refresh_directory_icons(
3434
directory_paths: Vec<String>,
3535
extensions: Vec<String>,
3636
use_app_icons_for_documents: bool,
37-
) -> HashMap<String, String> {
38-
blocking_with_timeout(ICONS_TIMEOUT, HashMap::new(), move || {
37+
) -> TimedOut<HashMap<String, String>> {
38+
blocking_with_timeout_flag(ICONS_TIMEOUT, HashMap::new(), move || {
3939
icons::refresh_icons_for_directory(directory_paths, extensions, use_app_icons_for_documents)
4040
})
4141
.await

0 commit comments

Comments
 (0)