Skip to content

Commit b365076

Browse files
committed
Bulk-skip pre-known conflicts under Skip-all for copy and move
TransferDialog already runs a pre-flight `scan_for_conflicts` and surfaces the result as "N files already exist". Previously that information was thrown away once the user clicked Copy: the backend re-discovered each conflict serially via per-file `dest.get_metadata` between (slow) copies, so under Skip-all the progress bar trickled by 1 per conflict instead of jumping to N immediately. For a 1046-file MTP→SMB copy with ~100 scattered conflicts this looked like the bar was stuck for many minutes while iteration crept through copies in between. Now the FE forwards the full conflicting-name list as `preKnownConflicts` on `VolumeCopyConfig` / `WriteOperationConfig`. The BE builds a `HashSet<PathBuf>` of source paths whose filename matches and, under Skip mode, bulk-accounts them in one shot before the main loop: bumps `files_done`/`bytes_done`, emits one progress event, and the main loop `continue`s past them. Other modes (Stop / Overwrite / Rename) ignore the list — Stop still emits per-file conflict events, Overwrite still overwrites. Three paths now apply it: `copy_volumes_with_progress` (cross-volume copy), `move_between_volumes` + `move_within_same_volume` (cross-volume / same-volume move), and `copy_files_with_progress` (local↔local copy via the short-circuit in `copy_between_volumes`). Bundled together because they share the same FE wire and the same data-safety pattern: - The `continue` is positioned before any destructive call (no `dest` write, no `source.delete()`), so skipped files are untouched on both sides. - Move-side per-iter `Skip` arm also picks up a latent fix: previously it `continue`d without bumping `files_done`, so the bar stalled through a run of conflicts even when "Skip all" was chosen via Stop's per-file dialog. `resolve_volume_conflict` now accepts `source_size_hint` / `dest_size_hint`. The copy path supplies real hints (source from `source_hints`, dest from `dest_meta`); the move path passes `None` for source (no scan phase) but supplies the dest hint. Before the hints, an MTP source's `scan_for_copy` listed the whole parent dir per conflict — an 18s stall before the very first conflict prompt on a 1046-file directory. TransferDialog now displays the true conflict count instead of capping at `maxConflictsToShow` (the cap was only meaningful for the per-row conflict list, which this dialog doesn't actually render). The "100+" misdirection is gone. Tests (in `volume_copy_tests.rs`): - `test_pre_known_conflicts_are_bulk_skipped_upfront` — happy path, concurrent loop - `test_stop_mode_does_not_bulk_skip_pre_known_conflicts` — Stop still emits per-file prompts - `test_pre_known_conflicts_ignored_outside_skip_mode` — Overwrite proceeds with overwrite - `test_pre_known_conflicts_with_stale_entries_is_safe` — stale or non-matching names can't crash or skip the wrong files - `test_pre_known_conflicts_bulk_skip_on_real_local_volumes` — `LocalPosixVolume` on tmpfile (catches `InMemoryVolume`-only assumptions) - `test_skipped_files_count_toward_progress` — per-iter skip accounting (Bug 2 from earlier in this thread) - `test_stop_conflict_does_not_rescan_source_when_hint_provided` — size-hint path doesn't re-scan
1 parent f052465 commit b365076

15 files changed

Lines changed: 1249 additions & 44 deletions

File tree

Cargo.lock

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

apps/desktop/src-tauri/src/file_system/write_operations/copy.rs

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ use super::helpers::{
1515
find_unique_name, is_same_file, resolve_conflict, run_cancellable, spawn_async_sync, validate_disk_space,
1616
validate_path_length,
1717
};
18-
use super::scan::{SourceItemTracker, handle_dry_run, scan_sources, take_cached_scan_result};
18+
use super::scan::{SourceItemTracker, handle_dry_run, scan_sources, take_cached_scan_result, top_level_source_path};
1919
use super::state::{
2020
CopyTransaction, OperationIntent, WriteOperationState, is_cancelled, load_intent, update_operation_status,
2121
};
@@ -81,7 +81,7 @@ pub(super) fn copy_files_with_progress(
8181
}
8282

8383
// Phase 1: Scan (or reuse cached preview results)
84-
let scan_result = if let Some(preview_id) = &config.preview_id {
84+
let mut scan_result = if let Some(preview_id) = &config.preview_id {
8585
// Try to reuse cached scan results from preview.
8686
// Volume scans (MTP, etc.) cache aggregate stats only (empty `files` list).
8787
// This per-file copy path needs the file list, so treat an empty-files cache
@@ -179,6 +179,74 @@ pub(super) fn copy_files_with_progress(
179179
scan_result.total_bytes,
180180
);
181181

182+
// Bulk-skip pre-known conflicts (Skip mode only). For local↔local, the
183+
// per-file `get_metadata` is cheap (microseconds), so the user-facing bug
184+
// is much milder than the cross-volume case — but we still want the bar
185+
// to reflect the user's "Skip all" decision immediately. The set is keyed
186+
// by the absolute top-level source path (matching what
187+
// `top_level_source_path(file_info)` returns).
188+
let pre_skip_top_levels: HashSet<PathBuf> =
189+
if config.conflict_resolution == ConflictResolution::Skip && !config.pre_known_conflicts.is_empty() {
190+
let names: HashSet<&str> = config.pre_known_conflicts.iter().map(String::as_str).collect();
191+
sources
192+
.iter()
193+
.filter(|p| {
194+
p.file_name()
195+
.and_then(|n| n.to_str())
196+
.map(|n| names.contains(n))
197+
.unwrap_or(false)
198+
})
199+
.cloned()
200+
.collect()
201+
} else {
202+
HashSet::new()
203+
};
204+
205+
if !pre_skip_top_levels.is_empty() {
206+
let mut skipped_count = 0usize;
207+
let mut skipped_bytes = 0u64;
208+
scan_result.files.retain(|fi| {
209+
let top = top_level_source_path(fi);
210+
if pre_skip_top_levels.contains(&top) {
211+
skipped_count += 1;
212+
skipped_bytes += fi.size;
213+
false
214+
} else {
215+
true
216+
}
217+
});
218+
files_done = skipped_count;
219+
bytes_done = skipped_bytes;
220+
log::info!(
221+
"copy_files_with_progress: bulk-skipping {} files ({} bytes) for {} pre-known conflicting top-level sources",
222+
skipped_count,
223+
skipped_bytes,
224+
pre_skip_top_levels.len()
225+
);
226+
state.emit_progress_via_app(
227+
app,
228+
WriteProgressEvent::new(
229+
operation_id.to_string(),
230+
WriteOperationType::Copy,
231+
WriteOperationPhase::Copying,
232+
None,
233+
files_done,
234+
scan_result.file_count,
235+
bytes_done,
236+
scan_result.total_bytes,
237+
),
238+
);
239+
update_operation_status(
240+
operation_id,
241+
WriteOperationPhase::Copying,
242+
None,
243+
files_done,
244+
scan_result.file_count,
245+
bytes_done,
246+
scan_result.total_bytes,
247+
);
248+
}
249+
182250
log::debug!(
183251
"copy_files_with_progress: starting copy loop for operation_id={}, {} files",
184252
operation_id,

apps/desktop/src-tauri/src/file_system/write_operations/types.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,10 @@ pub struct WriteOperationConfig {
567567
/// Maximum number of conflicts to include in DryRunResult (default: 100)
568568
#[serde(default = "default_max_conflicts_to_show")]
569569
pub max_conflicts_to_show: usize,
570+
/// Source filenames already known to conflict at the destination. See
571+
/// `VolumeCopyConfig::pre_known_conflicts` for the full rationale.
572+
#[serde(default)]
573+
pub pre_known_conflicts: Vec<String>,
570574
}
571575

572576
impl Default for WriteOperationConfig {
@@ -580,6 +584,7 @@ impl Default for WriteOperationConfig {
580584
sort_order: SortOrder::default(),
581585
preview_id: None,
582586
max_conflicts_to_show: default_max_conflicts_to_show(),
587+
pre_known_conflicts: Vec::new(),
583588
}
584589
}
585590
}
@@ -779,6 +784,16 @@ pub struct VolumeCopyConfig {
779784
/// Preview scan ID to reuse cached scan results (from start_scan_preview).
780785
#[serde(default)]
781786
pub preview_id: Option<String>,
787+
/// Source filenames already known to conflict at the destination (from the
788+
/// pre-flight `scan_for_conflicts` call). When `conflict_resolution == Skip`,
789+
/// the copy pipeline bulk-skips these upfront so the progress bar jumps to
790+
/// reflect them immediately, rather than discovering each one serially via
791+
/// per-file `get_metadata` stats while non-conflict copies run in between.
792+
/// Ignored for other resolution modes (Stop still prompts; Overwrite still
793+
/// proceeds normally). Empty if the FE didn't pre-scan or found no
794+
/// conflicts.
795+
#[serde(default)]
796+
pub pre_known_conflicts: Vec<String>,
782797
}
783798

784799
impl Default for VolumeCopyConfig {
@@ -788,6 +803,7 @@ impl Default for VolumeCopyConfig {
788803
conflict_resolution: ConflictResolution::Stop,
789804
max_conflicts_to_show: 100,
790805
preview_id: None,
806+
pre_known_conflicts: Vec::new(),
791807
}
792808
}
793809
}
@@ -799,6 +815,7 @@ impl From<&WriteOperationConfig> for VolumeCopyConfig {
799815
conflict_resolution: config.conflict_resolution,
800816
max_conflicts_to_show: config.max_conflicts_to_show,
801817
preview_id: config.preview_id.clone(),
818+
pre_known_conflicts: config.pre_known_conflicts.clone(),
802819
}
803820
}
804821
}

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

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,16 @@ pub(super) async fn resolve_volume_conflict(
2929
operation_id: &str,
3030
state: &Arc<WriteOperationState>,
3131
apply_to_all_resolution: &mut Option<ConflictResolution>,
32+
// Size hints for the conflict dialog. `Some` skips a `scan_for_copy` call
33+
// on that side. The copy path already has both: source size in
34+
// `source_hints` (from the cached preview scan), dest size in `dest_meta`
35+
// from the stat just done by the caller. Without these hints, an MTP
36+
// source means listing the parent directory of `source_path` to find one
37+
// entry's size — 18 s for /DCIM/Camera with 1046 photos when the listing
38+
// cache has lapsed. The move path doesn't have a scan phase, so it still
39+
// falls through to `scan_for_copy` for unknown hints.
40+
source_size_hint: Option<u64>,
41+
dest_size_hint: Option<u64>,
3242
) -> Result<Option<PathBuf>, WriteOperationError> {
3343
// Determine effective conflict resolution
3444
let resolution = if let Some(saved_resolution) = apply_to_all_resolution {
@@ -41,16 +51,24 @@ pub(super) async fn resolve_volume_conflict(
4151
match resolution {
4252
ConflictResolution::Stop => {
4353
// Need to prompt user - gather metadata for the conflict event
44-
let source_scan = source_volume.scan_for_copy(source_path).await.ok();
45-
let source_size = source_scan.as_ref().map(|s| s.total_bytes).unwrap_or(0);
54+
let source_size = if let Some(s) = source_size_hint {
55+
s
56+
} else {
57+
let source_scan = source_volume.scan_for_copy(source_path).await.ok();
58+
source_scan.as_ref().map(|s| s.total_bytes).unwrap_or(0)
59+
};
4660

47-
// Try to get destination size by scanning (best effort)
48-
let dest_size = dest_volume
49-
.scan_for_copy(dest_path)
50-
.await
51-
.ok()
52-
.map(|s| s.total_bytes)
53-
.unwrap_or(0);
61+
// Try to get destination size; prefer the hint to avoid a scan.
62+
let dest_size = if let Some(s) = dest_size_hint {
63+
s
64+
} else {
65+
dest_volume
66+
.scan_for_copy(dest_path)
67+
.await
68+
.ok()
69+
.map(|s| s.total_bytes)
70+
.unwrap_or(0)
71+
};
5472

5573
// We can't easily get modification times from Volume trait, so use None
5674
let source_modified: Option<i64> = None;

0 commit comments

Comments
 (0)