Skip to content

Commit 25f2b26

Browse files
vdavidvdavid
committed
MTP: Add E2E tests, fix the bugs found by them
Fix rename conflicts, paste guard, copy config, and add E2E tests - Make `check_rename_validity` volume-aware: accepts `volumeId`, uses `Volume::get_metadata()` for conflict detection on non-local volumes. MTP renames now show the same conflict dialog as local FS. Removes the `isMtp` guard in `rename-operations.ts`. - Fix `copyBetweenVolumes`/`moveBetweenVolumes` sending `config: {}` instead of `null` when no config provided — serde rejected the empty object (missing `conflictResolution`), causing "Copy failed" dialog. - Fix `pasteFromClipboard` checking clipboard before MTP volume — showed "No files on clipboard" instead of "Use F5 to copy files to MTP devices". Reordered so MTP rejection is checked first. - Add `rescan_virtual_mtp` Tauri command (feature-gated behind `virtual-mtp`) calling mtp-rs 0.8.0's new `rescan_virtual_device()`. Replaces the 2.5s sleep in `beforeEach` with a synchronous rescan — MTP test suite drops from 2.9min to 1.9min. - Add 8 new MTP E2E tests: delete (permanent dialog, multi-file, recursive folder), move (MTP↔local), clipboard rejection toasts (Cmd+C/X/V), rename conflict. - Add `DCIM/Burst/burst-001.jpg` to MTP fixtures for recursive delete testing. - Bump mtp-rs to 0.8.0.
1 parent d15c55f commit 25f2b26

14 files changed

Lines changed: 901 additions & 531 deletions

File tree

‎Cargo.lock‎

Lines changed: 393 additions & 467 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎apps/desktop/src-tauri/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ mime_guess = "2"
109109

110110
[target.'cfg(any(target_os = "macos", target_os = "linux"))'.dependencies]
111111
# MTP (Android device) support via pure Rust implementation
112-
mtp-rs = "0.7.2"
112+
mtp-rs = "0.8.0"
113113
# USB hotplug detection for MTP device watcher
114114
nusb = "0.2.3"
115115
bytes = "1"

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,3 +320,14 @@ pub async fn scan_mtp_for_copy(
320320
total_bytes: result.total_bytes,
321321
})
322322
}
323+
324+
/// Forces the virtual MTP device to rescan its backing directories, syncing
325+
/// its in-memory object tree with the actual filesystem. Call after recreating
326+
/// test fixtures to avoid sleeping for the file watcher.
327+
///
328+
/// Only available with `--features virtual-mtp`. Returns (added, removed) counts.
329+
#[cfg(feature = "virtual-mtp")]
330+
#[tauri::command]
331+
pub fn rescan_virtual_mtp() -> Result<(usize, usize), String> {
332+
crate::mtp::virtual_device::rescan_virtual_device().ok_or_else(|| "Virtual MTP device not found".to_string())
333+
}

‎apps/desktop/src-tauri/src/commands/rename.rs‎

Lines changed: 63 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -73,17 +73,23 @@ pub struct ConflictFileInfo {
7373

7474
/// Validates a new filename and checks for conflicts in the same directory.
7575
/// Uses inode comparison to detect case-only renames (valid on case-insensitive APFS).
76+
/// When `volume_id` is provided and not `"root"`, uses the Volume trait for conflict detection
77+
/// (needed for MTP and other non-local volumes).
7678
#[tauri::command]
7779
pub async fn check_rename_validity(
7880
dir: String,
7981
old_name: String,
8082
new_name: String,
83+
volume_id: Option<String>,
8184
) -> Result<RenameValidityResult, IpcError> {
8285
let expanded_dir = expand_tilde(&dir);
86+
let volume_id_str = volume_id.unwrap_or_else(|| "root".to_string());
8387

8488
tokio::time::timeout(
8589
Duration::from_secs(2),
86-
tokio::task::spawn_blocking(move || check_rename_validity_sync(&expanded_dir, &old_name, &new_name)),
90+
tokio::task::spawn_blocking(move || {
91+
check_rename_validity_sync(&expanded_dir, &old_name, &new_name, &volume_id_str)
92+
}),
8793
)
8894
.await
8995
.map_err(|_| IpcError::timeout())?
@@ -222,7 +228,12 @@ fn check_macos_flags(path: &Path) -> Result<(), String> {
222228
}
223229

224230
/// Synchronous validity check implementation.
225-
fn check_rename_validity_sync(dir: &str, old_name: &str, new_name: &str) -> Result<RenameValidityResult, String> {
231+
fn check_rename_validity_sync(
232+
dir: &str,
233+
old_name: &str,
234+
new_name: &str,
235+
volume_id: &str,
236+
) -> Result<RenameValidityResult, String> {
226237
use crate::file_system::validation::{validate_filename, validate_path_length};
227238

228239
let trimmed = new_name.trim();
@@ -252,15 +263,29 @@ fn check_rename_validity_sync(dir: &str, old_name: &str, new_name: &str) -> Resu
252263

253264
// Check for conflict: does a sibling with this name already exist?
254265
let old_path = PathBuf::from(dir).join(old_name);
255-
let conflict_info = check_sibling_conflict(&old_path, &new_path);
256-
257-
Ok(RenameValidityResult {
258-
valid: true,
259-
error: None,
260-
has_conflict: conflict_info.0,
261-
is_case_only_rename: conflict_info.1,
262-
conflict: conflict_info.2,
263-
})
266+
267+
if volume_id != "root" {
268+
// Non-local volume: use Volume trait for conflict detection
269+
let conflict_info = check_sibling_conflict_via_volume(volume_id, &new_path);
270+
Ok(RenameValidityResult {
271+
valid: true,
272+
error: None,
273+
has_conflict: conflict_info.0,
274+
// MTP is case-sensitive, no case-only rename ambiguity
275+
is_case_only_rename: false,
276+
conflict: conflict_info.1,
277+
})
278+
} else {
279+
// Local filesystem: use symlink_metadata with inode comparison
280+
let conflict_info = check_sibling_conflict(&old_path, &new_path);
281+
Ok(RenameValidityResult {
282+
valid: true,
283+
error: None,
284+
has_conflict: conflict_info.0,
285+
is_case_only_rename: conflict_info.1,
286+
conflict: conflict_info.2,
287+
})
288+
}
264289
}
265290

266291
/// Checks if a file with `new_path` exists and whether it's the same inode as `old_path`
@@ -325,6 +350,28 @@ fn check_sibling_conflict(_old_path: &Path, new_path: &Path) -> (bool, bool, Opt
325350
(true, false, Some(conflict))
326351
}
327352

353+
/// Checks if a file with `new_path` exists on a non-local volume using the Volume trait's `get_metadata`.
354+
fn check_sibling_conflict_via_volume(volume_id: &str, new_path: &Path) -> (bool, Option<ConflictFileInfo>) {
355+
let volume = match crate::file_system::get_volume_manager().get(volume_id) {
356+
Some(v) => v,
357+
None => return (false, None),
358+
};
359+
360+
let entry = match volume.get_metadata(new_path) {
361+
Ok(e) => e,
362+
Err(_) => return (false, None), // No conflict — file doesn't exist
363+
};
364+
365+
let conflict = ConflictFileInfo {
366+
name: entry.name,
367+
size: entry.size.unwrap_or(0),
368+
modified: entry.modified_at.map(|t| t as i64),
369+
is_directory: entry.is_directory,
370+
};
371+
372+
(true, Some(conflict))
373+
}
374+
328375
#[cfg(test)]
329376
mod tests {
330377
use super::*;
@@ -371,7 +418,7 @@ mod tests {
371418
let tmp = create_test_dir("rename_valid_ok");
372419
let dir = tmp.to_string_lossy().to_string();
373420
fs::write(tmp.join("old.txt"), "content").unwrap();
374-
let result = check_rename_validity(dir, "old.txt".to_string(), "new.txt".to_string()).await;
421+
let result = check_rename_validity(dir, "old.txt".to_string(), "new.txt".to_string(), None).await;
375422
assert!(result.is_ok());
376423
let check = result.unwrap();
377424
assert!(check.valid);
@@ -385,7 +432,7 @@ mod tests {
385432
async fn test_check_rename_validity_empty_name() {
386433
let tmp = create_test_dir("rename_valid_empty");
387434
let dir = tmp.to_string_lossy().to_string();
388-
let result = check_rename_validity(dir, "old.txt".to_string(), " ".to_string()).await;
435+
let result = check_rename_validity(dir, "old.txt".to_string(), " ".to_string(), None).await;
389436
assert!(result.is_ok());
390437
let check = result.unwrap();
391438
assert!(!check.valid);
@@ -397,7 +444,7 @@ mod tests {
397444
async fn test_check_rename_validity_slash_in_name() {
398445
let tmp = create_test_dir("rename_valid_slash");
399446
let dir = tmp.to_string_lossy().to_string();
400-
let result = check_rename_validity(dir, "old.txt".to_string(), "foo/bar".to_string()).await;
447+
let result = check_rename_validity(dir, "old.txt".to_string(), "foo/bar".to_string(), None).await;
401448
assert!(result.is_ok());
402449
let check = result.unwrap();
403450
assert!(!check.valid);
@@ -410,7 +457,7 @@ mod tests {
410457
let dir = tmp.to_string_lossy().to_string();
411458
fs::write(tmp.join("old.txt"), "old content").unwrap();
412459
fs::write(tmp.join("existing.txt"), "existing content").unwrap();
413-
let result = check_rename_validity(dir, "old.txt".to_string(), "existing.txt".to_string()).await;
460+
let result = check_rename_validity(dir, "old.txt".to_string(), "existing.txt".to_string(), None).await;
414461
assert!(result.is_ok());
415462
let check = result.unwrap();
416463
assert!(check.valid);
@@ -430,7 +477,7 @@ mod tests {
430477
let dir = tmp.to_string_lossy().to_string();
431478
fs::write(tmp.join("MyFile.txt"), "content").unwrap();
432479
// On case-insensitive APFS, "myfile.txt" resolves to the same inode as "MyFile.txt"
433-
let result = check_rename_validity(dir, "MyFile.txt".to_string(), "myfile.txt".to_string()).await;
480+
let result = check_rename_validity(dir, "MyFile.txt".to_string(), "myfile.txt".to_string(), None).await;
434481
assert!(result.is_ok());
435482
let check = result.unwrap();
436483
assert!(check.valid);

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,8 @@ pub fn run() {
709709
commands::mtp::move_mtp_object,
710710
#[cfg(any(target_os = "macos", target_os = "linux"))]
711711
commands::mtp::scan_mtp_for_copy,
712+
#[cfg(feature = "virtual-mtp")]
713+
commands::mtp::rescan_virtual_mtp,
712714
#[cfg(not(any(target_os = "macos", target_os = "linux")))]
713715
stubs::mtp::list_mtp_devices,
714716
#[cfg(not(any(target_os = "macos", target_os = "linux")))]

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,20 @@ pub fn setup_virtual_mtp_device() -> u64 {
9090

9191
location_id
9292
}
93+
94+
/// Serial number of the virtual device, used to look it up for rescan.
95+
pub const VIRTUAL_DEVICE_SERIAL: &str = "cmdr-e2e-virtual";
96+
97+
/// Forces the virtual MTP device to rescan its backing directories, syncing
98+
/// its in-memory object tree with the actual filesystem state.
99+
///
100+
/// Call this after recreating test fixtures to avoid waiting for the file watcher.
101+
/// Returns the number of objects added and removed, or None if the device wasn't found.
102+
pub fn rescan_virtual_device() -> Option<(usize, usize)> {
103+
let summary = mtp_rs::rescan_virtual_device(VIRTUAL_DEVICE_SERIAL)?;
104+
info!(
105+
"Virtual MTP device rescan: {} added, {} removed",
106+
summary.added, summary.removed
107+
);
108+
Some((summary.added as usize, summary.removed as usize))
109+
}

‎apps/desktop/src/lib/file-explorer/pane/DualPaneExplorer.svelte‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1532,19 +1532,21 @@
15321532
/** Pastes files from the system clipboard into the current directory. */
15331533
export async function pasteFromClipboard(forceMove: boolean) {
15341534
try {
1535+
// Check MTP before reading clipboard — MTP paste is always rejected,
1536+
// no point reading the system clipboard just to reject it.
1537+
const volumeId = getPaneVolumeId(focusedPane)
1538+
if (volumeId.startsWith('mtp-')) {
1539+
addToast('Use F5 to copy files to MTP devices')
1540+
return
1541+
}
1542+
15351543
const result = await readClipboardFiles()
15361544
15371545
if (result.paths.length === 0) {
15381546
addToast('No files on the clipboard. Copy files first with \u2318C.')
15391547
return
15401548
}
15411549
1542-
const volumeId = getPaneVolumeId(focusedPane)
1543-
if (volumeId.startsWith('mtp-')) {
1544-
addToast('Use F5 to copy files to MTP devices')
1545-
return
1546-
}
1547-
15481550
const operationType: TransferOperationType = result.isCut || forceMove ? 'move' : 'copy'
15491551
const destPath = getPanePath(focusedPane)
15501552
const { sortBy, sortOrder } = getPaneSort(focusedPane)

‎apps/desktop/src/lib/file-explorer/pane/rename-flow.svelte.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,9 @@ export function createRenameFlow(deps: RenameFlowDeps) {
159159
renameSiblingNames = names
160160
})
161161

162-
// Skip permission check for MTP volumes — it uses symlink_metadata
163-
// which doesn't work on MTP virtual paths.
162+
// Skip permission check for MTP volumes — checkRenamePermission uses
163+
// symlink_metadata and Unix access() which don't work on MTP virtual paths.
164+
// The validity check (conflict detection) IS volume-aware and runs for all volumes.
164165
const currentVolumeId = deps.getVolumeId()
165166
if (!currentVolumeId.startsWith('mtp-')) {
166167
void checkPermission(entry.path).then((errorMsg) => {

‎apps/desktop/src/lib/file-explorer/rename/CLAUDE.md‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,11 @@ read-only volume. Renaming isn't possible here."
6767
- Byte limits (255 for name, 1024 for path)
6868
- Extension change vs setting
6969

70-
**Backend (authoritative)**: `validation.rs` + `check_rename_validity` Tauri command. Uses inode comparison to detect
71-
case-only renames (same dev+ino on case-insensitive APFS).
70+
**Backend (authoritative)**: `validation.rs` + `check_rename_validity` Tauri command. Accepts an optional `volumeId`:
71+
72+
- Local FS (`volumeId` is `None` or `"root"`): uses `symlink_metadata` + inode comparison for case-only rename detection
73+
- Non-local volumes (MTP, etc.): uses `Volume::get_metadata()` for conflict detection, `is_case_only_rename` is always
74+
`false` (MTP is case-sensitive)
7275

7376
Both use platform-dependent logic with TODOs for future OSes.
7477

@@ -120,9 +123,10 @@ trimmed value.
120123
- **App-level shortcut suppression**: While rename active, Cmd+C/A/Z/X/V work as text editing shortcuts (not app
121124
commands). Implemented by setting flag in keyboard handler (same mechanism as dialogs). Other shortcuts (Cmd+O, arrow
122125
keys, etc.) are suppressed.
123-
- **MTP volume ID threading**: `rename-operations.ts` passes `volumeId` through to `renameFile` and
124-
`checkRenamePermission`. Permission checks are skipped for non-root volumes (MTP doesn't support Unix `access()`
125-
checks).
126+
- **MTP volume ID threading**: `rename-operations.ts` passes `volumeId` through to `renameFile`, `checkRenameValidity`,
127+
and `checkRenamePermission`. Validity checks (conflict detection) work for all volumes via the Volume trait.
128+
Permission checks are still skipped for MTP volumes (they use Unix `access()` which doesn't work on MTP virtual
129+
paths).
126130
- **Refresh timing**: File watcher event arrives asynchronously. `moveCursorToNewFolder()` pattern: subscribe to
127131
`directory-diff`, wait 50ms after event for listing cache update, then query `findFileIndex()`. Cleanup listener after
128132
3s timeout.

‎apps/desktop/src/lib/file-explorer/rename/rename-operations.ts‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -62,27 +62,22 @@ export async function executeRenameSave(
6262
}
6363
}
6464

65-
// Skip permission and validity checks for MTP volumes — they use symlink_metadata
66-
// which doesn't work on MTP virtual paths.
67-
const isMtp = volumeId?.startsWith('mtp-') ?? false
68-
69-
if (!isMtp) {
70-
// Backend validity check (authoritative, checks conflicts via inode comparison)
71-
let validity: RenameValidityResult
72-
try {
73-
validity = await checkRenameValidity(target.parentPath, target.originalName, trimmedName)
74-
} catch (e) {
75-
return { type: 'error', message: getIpcErrorMessage(e) }
76-
}
65+
// Backend validity check (authoritative, checks conflicts via inode comparison on local FS,
66+
// or Volume trait's get_metadata on MTP and other non-local volumes)
67+
let validity: RenameValidityResult
68+
try {
69+
validity = await checkRenameValidity(target.parentPath, target.originalName, trimmedName, volumeId)
70+
} catch (e) {
71+
return { type: 'error', message: getIpcErrorMessage(e) }
72+
}
7773

78-
if (!validity.valid) {
79-
return { type: 'error', message: validity.error?.message ?? 'Invalid filename' }
80-
}
74+
if (!validity.valid) {
75+
return { type: 'error', message: validity.error?.message ?? 'Invalid filename' }
76+
}
8177

82-
// Conflict detected (and not a case-only rename of the same file)
83-
if (validity.hasConflict && !validity.isCaseOnlyRename) {
84-
return { type: 'conflict', validity }
85-
}
78+
// Conflict detected (and not a case-only rename of the same file)
79+
if (validity.hasConflict && !validity.isCaseOnlyRename) {
80+
return { type: 'conflict', validity }
8681
}
8782

8883
// Perform the rename

0 commit comments

Comments
 (0)