Skip to content

Commit b096659

Browse files
vdavidvdavid
committed
Volume selector: push-based, fix race conditions
Backend: - `volume_broadcast.rs` (new): single `volumes-changed` event replaces multiple separate events. 150ms debounce, 2s timeout, cross-platform - `mtp/watcher.rs`: auto-connects MTP devices on USB hotplug instead of letting the frontend orchestrate - `volumes/mod.rs`: removed 5s `LOCATIONS_CACHE`. Added `get_fsid()` guard in `get_attached_volumes()` to skip volumes whose mount hasn't settled - `volumes/watcher.rs`: `spawn_mount_settle_watcher()` polls fsid up to 10x at 1s intervals after a mount, re-broadcasts when metadata is ready - `mtp/connection/mod.rs` + `directory_ops.rs`: emit `volumes-changed` after MTP connect/disconnect - `volumes_linux/watcher.rs`: emit `volumes-changed` after mount/unmount Frontend: - `volume-store.svelte.ts` (new): single source of truth for volumes. Subscribes to `volumes-changed`, tracks `timedOut`/`refreshing`/`retryFailed` - `VolumeBreadcrumb.svelte`: pure presentational — reads from store, no fetching or event listeners - `DualPaneExplorer.svelte`: reads from store, removed ~6 `listVolumes()` calls and 4 event listeners - `mtp-store.svelte.ts`: passive consumer — backend handles all MTP connection orchestration - `DialogManager.svelte` + `TransferDialog.svelte`: read volumes from store instead of prop - Removed dead `onMtpDeviceDetected`/`onMtpDeviceRemoved` code - Fixed 3 pre-existing ESLint errors - Updated tests for new passive model Docs: - Add info about cmdr_lib:: prefix for RUST_LOG
1 parent 271a2da commit b096659

File tree

29 files changed

+726
-449
lines changed

29 files changed

+726
-449
lines changed

‎AGENTS.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ Always use the checker script for compilation, linting, formatting, and tests. I
6767
`~/Library/Application Support/com.veszelovszki.cmdr-dev/`. Set by `resolved_app_data_dir()` in
6868
`src-tauri/src/config.rs`.
6969
- **Logging**: Frontend and backend logs appear together in terminal and in `~/Library/Logs/com.veszelovszki.cmdr/`.
70-
Full reference with `RUST_LOG` recipes: [docs/tooling/logging.md](docs/tooling/logging.md).
70+
**Read [docs/tooling/logging.md](docs/tooling/logging.md) before using `RUST_LOG`** — it has copy-paste recipes for
71+
every subsystem. Key gotcha: the Rust library target is `cmdr_lib`, not `cmdr`. Use `RUST_LOG=cmdr_lib::module=debug`.
7172
- **Crash reports**: When the app crashes, it writes a crash file to the data dir (`crash-report.json` alongside
7273
`settings.json`). On next launch, the app detects this file and offers to send a crash report. See
7374
`src-tauri/src/crash_reporter/CLAUDE.md` for architecture details.

‎apps/desktop/coverage-allowlist.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,9 @@
242242
},
243243
"ui/ProgressBar.svelte": {
244244
"reason": "Pure UI component, no logic to test beyond rendering"
245+
},
246+
"stores/volume-store.svelte.ts": {
247+
"reason": "Depends on Tauri listen/invoke APIs, integration point between backend events and frontend state"
245248
}
246249
}
247250
}

‎apps/desktop/src-tauri/src/file_system/volume/in_memory.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -327,13 +327,14 @@ impl Volume for InMemoryVolume {
327327

328328
// Check if the path is a file
329329
if let Some(entry) = entries.get(&normalized)
330-
&& !entry.metadata.is_directory {
331-
return Ok(CopyScanResult {
332-
file_count: 1,
333-
dir_count: 0,
334-
total_bytes: entry.metadata.size.unwrap_or(0),
335-
});
336-
}
330+
&& !entry.metadata.is_directory
331+
{
332+
return Ok(CopyScanResult {
333+
file_count: 1,
334+
dir_count: 0,
335+
total_bytes: entry.metadata.size.unwrap_or(0),
336+
});
337+
}
337338

338339
// Recursively scan all descendants
339340
let mut file_count = 0;

‎apps/desktop/src-tauri/src/lib.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ pub mod search;
108108
mod settings;
109109
#[cfg(target_os = "macos")]
110110
mod updater;
111+
mod volume_broadcast;
111112
#[cfg(target_os = "macos")]
112113
mod volumes;
113114
#[cfg(target_os = "linux")]
@@ -300,6 +301,9 @@ pub fn run() {
300301
#[cfg(any(target_os = "macos", target_os = "linux"))]
301302
network::start_discovery(app.handle().clone());
302303

304+
// Initialize volume broadcast (must be before watchers so they can emit)
305+
volume_broadcast::init(app.handle());
306+
303307
// Start volume mount/unmount watcher
304308
#[cfg(target_os = "macos")]
305309
volumes::watcher::start_volume_watcher(app.handle());
@@ -312,9 +316,13 @@ pub fn run() {
312316
mtp::virtual_device::setup_virtual_mtp_device();
313317

314318
// Start MTP device hotplug watcher (Android device support)
319+
// This also auto-connects any devices already plugged in at startup
315320
#[cfg(any(target_os = "macos", target_os = "linux"))]
316321
mtp::start_mtp_watcher(app.handle());
317322

323+
// Emit initial volume list (after watchers start so MTP devices can connect)
324+
volume_broadcast::emit_volumes_changed_now();
325+
318326
// Load known network shares from disk
319327
#[cfg(any(target_os = "macos", target_os = "linux"))]
320328
network::known_shares::load_known_shares(app.handle());
@@ -716,6 +724,8 @@ pub fn run() {
716724
stubs::mtp::move_mtp_object,
717725
#[cfg(not(any(target_os = "macos", target_os = "linux")))]
718726
stubs::mtp::scan_mtp_for_copy,
727+
// Volume broadcast (cross-platform)
728+
volume_broadcast::refresh_volumes,
719729
// Volume commands (platform-specific)
720730
#[cfg(target_os = "macos")]
721731
commands::volumes::list_volumes,

‎apps/desktop/src-tauri/src/mtp/CLAUDE.md‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ On Linux, users may need udev rules for USB device permissions (see `resources/9
1111
| `mod.rs` | Re-exports public surface; module-level doc |
1212
| `types.rs` | `MtpDeviceInfo`, `MtpStorageInfo` — camelCase JSON via `serde(rename_all)` |
1313
| `discovery.rs` | `list_mtp_devices()` via `mtp_rs::MtpDevice::list_devices()`; device IDs formatted as `"mtp-{location_id}"` |
14-
| `watcher.rs` | `start_mtp_watcher()` — nusb hotplug watcher; 500 ms delay on connect before re-checking; emits `mtp-device-detected` / `mtp-device-removed` Tauri events |
14+
| `watcher.rs` | `start_mtp_watcher()` — nusb hotplug watcher; 500 ms delay on connect before re-checking; auto-connects detected devices via `MtpConnectionManager::connect()` and auto-disconnects removed ones |
1515
| `macos_workaround.rs` | macOS-only (`#[cfg(target_os = "macos")]`). Detects `ptpcamerad` via `ioreg`; exposes `PTPCAMERAD_WORKAROUND_COMMAND` (a bash one-liner) |
1616
| `connection/mod.rs` | `MtpConnectionManager` singleton (`LazyLock`); `DeviceEntry` map; `connect()` (idempotent, probes write capability, registers `MtpVolume`); `disconnect()` |
1717
| `connection/cache.rs` | `PathHandleCache` (path → MTP object handle), `ListingCache` (5 s TTL), `EventDebouncer` (500 ms per device) |
@@ -21,7 +21,7 @@ On Linux, users may need udev rules for USB device permissions (see `resources/9
2121
| `connection/file_ops.rs` | `download_file()`, `upload_file()` — emit `mtp-transfer-progress` Tauri events |
2222
| `connection/mutation_ops.rs` | `delete()` (recursive, children-first), `create_folder()`, `rename()`, `move_object()` — no copy+delete fallback |
2323
| `connection/bulk_ops.rs` | `scan_for_copy()`, `download_recursive()`, `upload_recursive()` — use `Box::pin` for async recursion |
24-
| `virtual_device.rs` | Virtual MTP device for E2E testing; creates backing dirs + registers device via `mtp-rs`. Gated behind `virtual-mtp` feature. |
24+
| `virtual_device.rs` | Virtual MTP device for E2E testing; creates backing dirs + registers device via `mtp-rs`. Gated behind `virtual-mtp` feature. Run with: `cd apps/desktop && pnpm tauri dev -c src-tauri/tauri.dev.json --features virtual-mtp` |
2525

2626
## Architecture / data flow
2727

@@ -30,22 +30,32 @@ USB plug-in
3030
→ nusb hotplug event (watcher.rs)
3131
→ 500 ms delay
3232
→ list_mtp_devices() (discovery.rs)
33-
→ emit mtp-device-detected
33+
→ auto_connect_device() (watcher.rs)
34+
→ MtpConnectionManager::connect()
35+
→ open_device() via MtpDeviceBuilder
36+
→ probe_write_capability() per storage
37+
→ register MtpVolume in global VolumeManager
38+
→ start_event_loop() per device
39+
→ emit mtp-device-connected
40+
→ broadcast::emit_volumes_changed()
3441
35-
Frontend calls connect_mtp_device
36-
→ MtpConnectionManager::connect()
37-
→ open_device() via MtpDeviceBuilder
38-
→ probe_write_capability() per storage
39-
→ register MtpVolume in global VolumeManager
40-
→ start_event_loop() per device
41-
→ emit mtp-device-connected
42+
USB unplug
43+
→ nusb hotplug event (watcher.rs)
44+
→ auto_disconnect_device() (watcher.rs)
45+
→ MtpConnectionManager::disconnect()
46+
→ emit mtp-device-disconnected
47+
→ broadcast::emit_volumes_changed()
4248
4349
Event loop (event_loop.rs)
4450
→ device.next_event()
4551
→ compute_diff()
4652
→ emit directory-diff (same format as local file watching)
4753
```
4854

55+
The frontend is a passive consumer: it subscribes to `volumes-changed` (for the volume picker)
56+
and `mtp-device-connected`/`mtp-device-disconnected` (for device connection state tracking).
57+
It never orchestrates MTP connections.
58+
4959
## Key patterns and gotchas
5060

5161
- **Device lock**: `Arc<tokio::sync::Mutex<MtpDevice>>` held for the entire USB I/O call (tokio's Mutex can be held across `.await` points, unlike `std::sync::Mutex`). Operations are serialized per device with a 30 s timeout (`MTP_TIMEOUT_SECS`). Holding the lock too long logs a warning.

‎apps/desktop/src-tauri/src/mtp/connection/directory_ops.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,9 @@ impl MtpConnectionManager {
633633
device_id
634634
);
635635
}
636+
637+
// Broadcast updated volume list (MTP volume removed)
638+
crate::volume_broadcast::emit_volumes_changed();
636639
} else {
637640
debug!(
638641
"handle_device_disconnected: device {} was not in registry (already cleaned up?)",

‎apps/desktop/src-tauri/src/mtp/connection/mod.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,9 @@ impl MtpConnectionManager {
351351
);
352352
}
353353

354+
// Broadcast updated volume list (includes new MTP volumes)
355+
crate::volume_broadcast::emit_volumes_changed();
356+
354357
info!(
355358
"MTP device connected: {} ({} storages)",
356359
device_id,
@@ -406,6 +409,9 @@ impl MtpConnectionManager {
406409
);
407410
}
408411

412+
// Broadcast updated volume list (MTP volume removed)
413+
crate::volume_broadcast::emit_volumes_changed();
414+
409415
info!("MTP device disconnected: {}", device_id);
410416
Ok(())
411417
}

‎apps/desktop/src-tauri/src/mtp/watcher.rs‎

Lines changed: 55 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
//! USB hotplug watcher for MTP devices.
22
//!
3-
//! Watches for USB device connect/disconnect events and emits Tauri events
4-
//! when MTP devices are detected or removed. Uses nusb's hotplug API.
3+
//! Watches for USB device connect/disconnect events via nusb's hotplug API.
4+
//! On detection, auto-connects devices and emits `mtp-device-connected` /
5+
//! `mtp-device-disconnected` events (via the connection manager). The frontend
6+
//! is a passive consumer — it never orchestrates connections.
57
68
use log::{debug, error, info, warn};
79
use nusb::hotplug::HotplugEvent;
810
use std::collections::HashSet;
911
use std::sync::{Mutex, OnceLock};
10-
use tauri::{AppHandle, Emitter};
12+
use tauri::AppHandle;
1113

1214
/// Global app handle for emitting events from the watcher
1315
static APP_HANDLE: OnceLock<AppHandle> = OnceLock::new();
@@ -18,36 +20,14 @@ static KNOWN_DEVICES: OnceLock<Mutex<HashSet<String>>> = OnceLock::new();
1820
/// Flag to indicate watcher has been started
1921
static WATCHER_STARTED: OnceLock<()> = OnceLock::new();
2022

21-
/// Payload for MTP device detected event
22-
#[derive(Clone, serde::Serialize)]
23-
#[serde(rename_all = "camelCase")]
24-
pub struct MtpDeviceDetectedPayload {
25-
/// The device ID
26-
pub device_id: String,
27-
/// Device name (if available)
28-
pub name: Option<String>,
29-
/// USB vendor ID
30-
pub vendor_id: u16,
31-
/// USB product ID
32-
pub product_id: u16,
33-
}
34-
35-
/// Payload for MTP device removed event
36-
#[derive(Clone, serde::Serialize)]
37-
#[serde(rename_all = "camelCase")]
38-
pub struct MtpDeviceRemovedPayload {
39-
/// The device ID
40-
pub device_id: String,
41-
}
42-
4323
/// Gets the current set of MTP devices using mtp-rs discovery.
4424
fn get_current_mtp_devices() -> HashSet<String> {
4525
let devices = super::list_mtp_devices();
4626
devices.into_iter().map(|d| d.id).collect()
4727
}
4828

4929
/// Checks for MTP device changes by comparing current state with known state.
50-
/// Emits events for newly detected and removed devices.
30+
/// Auto-connects newly detected devices and disconnects removed ones.
5131
fn check_for_device_changes() {
5232
let current_devices = get_current_mtp_devices();
5333

@@ -61,60 +41,62 @@ fn check_for_device_changes() {
6141
Err(_) => return,
6242
};
6343

64-
// Find newly detected devices
65-
for device_id in current_devices.difference(&known_guard) {
66-
debug!("MTP device detected: {}", device_id);
67-
emit_device_detected(device_id);
68-
}
44+
let new_devices: Vec<_> = current_devices.difference(&known_guard).cloned().collect();
45+
let removed_devices: Vec<_> = known_guard.difference(&current_devices).cloned().collect();
46+
47+
// Update known devices before async work to avoid re-triggering
48+
*known_guard = current_devices;
49+
drop(known_guard);
6950

70-
// Find removed devices
71-
for device_id in known_guard.difference(&current_devices) {
72-
debug!("MTP device removed: {}", device_id);
73-
emit_device_removed(device_id);
51+
// Auto-connect newly detected devices
52+
for device_id in new_devices {
53+
info!("MTP device detected, auto-connecting: {}", device_id);
54+
auto_connect_device(device_id);
7455
}
7556

76-
// Update known devices
77-
*known_guard = current_devices;
57+
// Disconnect removed devices
58+
for device_id in removed_devices {
59+
info!("MTP device removed, disconnecting: {}", device_id);
60+
auto_disconnect_device(device_id);
61+
}
7862
}
7963

80-
/// Emit a device detected event to the frontend.
81-
fn emit_device_detected(device_id: &str) {
82-
if let Some(app) = APP_HANDLE.get() {
83-
// Try to get full device info
84-
let devices = super::list_mtp_devices();
85-
let device_info = devices.iter().find(|d| d.id == device_id);
86-
87-
let payload = MtpDeviceDetectedPayload {
88-
device_id: device_id.to_string(),
89-
name: device_info.and_then(|d| d.product.clone()),
90-
vendor_id: device_info.map(|d| d.vendor_id).unwrap_or(0),
91-
product_id: device_info.map(|d| d.product_id).unwrap_or(0),
92-
};
93-
94-
if let Err(e) = app.emit("mtp-device-detected", payload) {
95-
error!("Failed to emit mtp-device-detected event: {}", e);
96-
} else {
97-
info!("Emitted mtp-device-detected for {}", device_id);
64+
/// Spawns an async task to connect a newly detected MTP device.
65+
fn auto_connect_device(device_id: String) {
66+
let app = APP_HANDLE.get().cloned();
67+
tauri::async_runtime::spawn(async move {
68+
let cm = super::connection_manager();
69+
match cm.connect(&device_id, app.as_ref()).await {
70+
Ok(info) => {
71+
info!(
72+
"Auto-connected MTP device: {} ({} storages)",
73+
device_id,
74+
info.storages.len()
75+
);
76+
}
77+
Err(e) => {
78+
// Connection errors (exclusive access, permission) are already
79+
// emitted as events by the connection manager
80+
warn!("Failed to auto-connect MTP device {}: {:?}", device_id, e);
81+
}
9882
}
99-
}
83+
});
10084
}
10185

102-
/// Emit a device removed event to the frontend.
103-
fn emit_device_removed(device_id: &str) {
104-
if let Some(app) = APP_HANDLE.get() {
105-
let payload = MtpDeviceRemovedPayload {
106-
device_id: device_id.to_string(),
107-
};
108-
109-
if let Err(e) = app.emit("mtp-device-removed", payload) {
110-
error!("Failed to emit mtp-device-removed event: {}", e);
111-
} else {
112-
info!("Emitted mtp-device-removed for {}", device_id);
86+
/// Spawns an async task to disconnect a removed MTP device.
87+
fn auto_disconnect_device(device_id: String) {
88+
let app = APP_HANDLE.get().cloned();
89+
tauri::async_runtime::spawn(async move {
90+
let cm = super::connection_manager();
91+
if let Err(e) = cm.disconnect(&device_id, app.as_ref()).await {
92+
// NotConnected is fine — device may not have been connected yet
93+
debug!("Disconnect for removed device {} returned: {:?}", device_id, e);
11394
}
114-
}
95+
});
11596
}
11697

11798
/// Starts the USB hotplug watcher for MTP devices.
99+
/// Also auto-connects any devices that are already plugged in at startup.
118100
/// Call this once at app initialization.
119101
pub fn start_mtp_watcher(app: &AppHandle) {
120102
// Only start once
@@ -141,6 +123,11 @@ pub fn start_mtp_watcher(app: &AppHandle) {
141123
initial_devices.len()
142124
);
143125

126+
// Auto-connect any devices already plugged in at startup
127+
for device_id in &initial_devices {
128+
auto_connect_device(device_id.clone());
129+
}
130+
144131
// Spawn the async hotplug watcher using Tauri's async runtime
145132
// (tokio::spawn doesn't work here as we're in a synchronous setup hook)
146133
let app_handle = app.clone();
@@ -193,30 +180,6 @@ async fn run_hotplug_watcher(_app: AppHandle) {
193180
mod tests {
194181
use super::*;
195182

196-
#[test]
197-
fn test_device_detected_payload_serialization() {
198-
let payload = MtpDeviceDetectedPayload {
199-
device_id: "mtp-336592896".to_string(),
200-
name: Some("Pixel 8".to_string()),
201-
vendor_id: 0x18d1,
202-
product_id: 0x4ee1,
203-
};
204-
let json = serde_json::to_string(&payload).unwrap();
205-
assert!(json.contains("deviceId"));
206-
assert!(json.contains("mtp-336592896"));
207-
assert!(json.contains("vendorId"));
208-
}
209-
210-
#[test]
211-
fn test_device_removed_payload_serialization() {
212-
let payload = MtpDeviceRemovedPayload {
213-
device_id: "mtp-336592896".to_string(),
214-
};
215-
let json = serde_json::to_string(&payload).unwrap();
216-
assert!(json.contains("deviceId"));
217-
assert!(json.contains("mtp-336592896"));
218-
}
219-
220183
#[test]
221184
fn test_get_current_mtp_devices() {
222185
// This test just verifies the function runs without panicking

0 commit comments

Comments
 (0)