Skip to content

Commit 27f2ff0

Browse files
vdavidvdavid
committed
Bugfix: MTP move conflicts no longer silently overwrite
- `move_between_volumes` (cross-volume move) now checks `dest_volume.get_metadata()` before `copy_single_path` and calls `resolve_volume_conflict` on hit — same pattern as the copy path. Skip = preserve both files; Overwrite = replace dest, delete source. - `move_within_same_volume` (same-volume rename) now checks for conflicts before `volume.rename()`. Previously, a name collision would either silently overwrite or produce a bare MTP error depending on device firmware. - Removed stale `#![allow(dead_code)]` and "Phase 5" TODOs from `volume_conflict.rs` and `volume_strategy.rs` — these modules are fully wired into Tauri commands.
1 parent 3bed65f commit 27f2ff0

File tree

4 files changed

+105
-12
lines changed

4 files changed

+105
-12
lines changed

‎apps/desktop/src-tauri/src/file_system/write_operations/CLAUDE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ network mounts, cross-filesystem moves, and name/path length limits.
2525
| `macos_copy.rs` | FFI to macOS `copyfile(3)`. Preserves xattrs, ACLs, resource forks, Finder metadata. Supports APFS `clonefile`. |
2626
| `linux_copy.rs` | Linux `copy_file_range(2)` with reflink support on btrfs/XFS. 4 MB chunks, cancellation between iterations. |
2727
| `chunked_copy.rs` | 1 MB chunked read/write — the default copy method for all non-APFS-clonefile copies on macOS and network copies on Linux. Checks cancellation between chunks. Copies xattrs, ACLs, timestamps. |
28-
| `volume_copy.rs`, `volume_conflict.rs`, `volume_strategy.rs` | Volume-to-volume copy (Local↔MTP abstraction). Publicly re-exported from `mod.rs` and at least partially wired up. |
28+
| `volume_copy.rs`, `volume_conflict.rs`, `volume_strategy.rs` | Volume-to-volume copy/move (Local↔MTP abstraction). Handles conflict detection, resolution (Stop/Skip/Overwrite/Rename), and progress. Wired into Tauri commands `copy_between_volumes` and `move_between_volumes`. |
2929
| `tests.rs`, `integration_test.rs` | Unit and integration tests. |
3030

3131
## Architecture / data flow
@@ -133,7 +133,7 @@ frontend's `handleError` removes all listeners on first receipt.
133133
threads (used for temp/backup file cleanup, not for user-visible rollback). If the network mount disconnects or the app
134134
exits, partial files or staging directories may remain on disk. These use the `.cmdr-` prefix, so they're recognizable.
135135

136-
**`volume_copy` path is incomplete.** The three `volume_*` files are Phase 5 work, but are publicly re-exported from `mod.rs` and at least partially wired up.
136+
**`volume_copy` path is fully wired up.** The three `volume_*` files are re-exported from `mod.rs` and called by the `copy_between_volumes` and `move_between_volumes` Tauri commands. Both copy and move operations support conflict detection and resolution (Stop/Skip/Overwrite/Rename) for all volume combinations (Local↔MTP, MTP↔MTP).
137137

138138
## Events emitted
139139

‎apps/desktop/src-tauri/src/file_system/write_operations/volume_conflict.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,6 @@
66
//! - Overwrite: Delete existing, return same path
77
//! - Rename: Find unique name like "file (1).txt"
88
9-
// TODO: Remove this once volume_copy is integrated into Tauri commands (Phase 5)
10-
#![allow(dead_code, reason = "Volume copy not yet integrated into Tauri commands")]
11-
129
use std::path::{Path, PathBuf};
1310
use std::sync::Arc;
1411

‎apps/desktop/src-tauri/src/file_system/write_operations/volume_copy.rs‎

Lines changed: 103 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,7 @@ pub async fn move_between_volumes(
567567
) -> Result<WriteOperationStartResult, WriteOperationError> {
568568
// Same volume — use native rename/move (instant for MTP)
569569
if Arc::ptr_eq(&source_volume, &dest_volume) {
570-
return move_within_same_volume(app, source_volume, source_paths, dest_path, &config).await;
570+
return move_within_same_volume(app, source_volume, source_paths, dest_path, config).await;
571571
}
572572

573573
// Both local — delegate to the battle-tested move implementation
@@ -631,6 +631,8 @@ pub async fn move_between_volumes(
631631

632632
// Copy+delete per file: on partial failure, already-moved files exist at dest,
633633
// remaining files stay at source. No data loss, but the move is partial.
634+
let mut apply_to_all_resolution: Option<ConflictResolution> = None;
635+
634636
for source_path in &source_paths {
635637
if super::state::is_cancelled(&state.intent) {
636638
return Err(WriteOperationError::Cancelled {
@@ -643,7 +645,55 @@ pub async fn move_between_volumes(
643645
message: "Invalid source path".to_string(),
644646
})?;
645647

646-
let dest_item = dest_path.join(file_name);
648+
let mut dest_item = dest_path.join(file_name);
649+
650+
// Check for conflict: does destination already exist?
651+
if let Ok(dest_meta) = dest_volume.get_metadata(&dest_item) {
652+
let source_is_dir = source_volume.is_directory(source_path).unwrap_or(false);
653+
let dest_is_dir = dest_meta.is_directory;
654+
655+
if source_is_dir && dest_is_dir {
656+
// Both are directories — merge, not a conflict
657+
log::debug!(
658+
"move_between_volumes: merging directories {} -> {}",
659+
source_path.display(),
660+
dest_item.display()
661+
);
662+
} else {
663+
log::debug!(
664+
"move_between_volumes: conflict detected at {} (source_is_dir={}, dest_is_dir={})",
665+
dest_item.display(),
666+
source_is_dir,
667+
dest_is_dir
668+
);
669+
670+
let resolved = resolve_volume_conflict(
671+
&source_volume,
672+
source_path,
673+
&dest_volume,
674+
&dest_item,
675+
&config,
676+
&app,
677+
&operation_id_for_spawn,
678+
&state,
679+
&mut apply_to_all_resolution,
680+
)?;
681+
682+
match resolved {
683+
None => {
684+
// Skip — don't copy and don't delete source
685+
log::debug!(
686+
"move_between_volumes: skipping {} due to conflict resolution",
687+
source_path.display()
688+
);
689+
continue;
690+
}
691+
Some(resolved_path) => {
692+
dest_item = resolved_path;
693+
}
694+
}
695+
}
696+
}
647697

648698
// Copy to destination
649699
let bytes = copy_single_path(&source_volume, source_path, &dest_volume, &dest_item, &state)
@@ -745,7 +795,7 @@ async fn move_within_same_volume(
745795
volume: Arc<dyn Volume>,
746796
source_paths: Vec<PathBuf>,
747797
dest_path: PathBuf,
748-
config: &VolumeCopyConfig,
798+
config: VolumeCopyConfig,
749799
) -> Result<WriteOperationStartResult, WriteOperationError> {
750800
let operation_id = Uuid::new_v4().to_string();
751801
let operation_id_for_spawn = operation_id.clone();
@@ -785,6 +835,7 @@ async fn move_within_same_volume(
785835
let mut bytes_moved = 0u64;
786836
let mut last_progress_time = Instant::now();
787837
let progress_interval = Duration::from_millis(progress_interval_ms);
838+
let mut apply_to_all_resolution: Option<ConflictResolution> = None;
788839

789840
for source_path in &source_paths {
790841
if super::state::is_cancelled(&state.intent) {
@@ -798,7 +849,55 @@ async fn move_within_same_volume(
798849
message: "Invalid source path".to_string(),
799850
})?;
800851

801-
let dest_item = dest_path.join(file_name);
852+
let mut dest_item = dest_path.join(file_name);
853+
854+
// Check for conflict: does destination already exist?
855+
if let Ok(dest_meta) = volume.get_metadata(&dest_item) {
856+
let source_is_dir = volume.is_directory(source_path).unwrap_or(false);
857+
let dest_is_dir = dest_meta.is_directory;
858+
859+
if source_is_dir && dest_is_dir {
860+
// Both are directories — merge, not a conflict
861+
log::debug!(
862+
"move_within_same_volume: merging directories {} -> {}",
863+
source_path.display(),
864+
dest_item.display()
865+
);
866+
} else {
867+
log::debug!(
868+
"move_within_same_volume: conflict detected at {} (source_is_dir={}, dest_is_dir={})",
869+
dest_item.display(),
870+
source_is_dir,
871+
dest_is_dir
872+
);
873+
874+
let resolved = resolve_volume_conflict(
875+
&volume,
876+
source_path,
877+
&volume,
878+
&dest_item,
879+
&config,
880+
&app,
881+
&operation_id_for_spawn,
882+
&state,
883+
&mut apply_to_all_resolution,
884+
)?;
885+
886+
match resolved {
887+
None => {
888+
// Skip — don't move this file
889+
log::debug!(
890+
"move_within_same_volume: skipping {} due to conflict resolution",
891+
source_path.display()
892+
);
893+
continue;
894+
}
895+
Some(resolved_path) => {
896+
dest_item = resolved_path;
897+
}
898+
}
899+
}
900+
}
802901

803902
let size = volume.get_metadata(source_path).ok().and_then(|m| m.size).unwrap_or(0);
804903

‎apps/desktop/src-tauri/src/file_system/write_operations/volume_strategy.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,6 @@
77
//! - MTP → MTP file: streaming transfer
88
//! - MTP → MTP directory: export to temp local, then import
99
10-
// TODO: Remove this once volume_copy is integrated into Tauri commands (Phase 5)
11-
#![allow(dead_code, reason = "Volume copy not yet integrated into Tauri commands")]
12-
1310
use std::path::Path;
1411
use std::sync::Arc;
1512

0 commit comments

Comments
 (0)