Skip to content

Commit 8856e01

Browse files
committed
Move dialog: restore the Size progress bar
- Cross-volume move (e.g. MTP→local) was shipping `bytes_total = 0` on every progress event, so the FE's `{#if bytesTotal > 0}` gate hid the Size bar — only the Files bar showed during the operation, even though `bytes_done` was being tracked correctly. Same-volume rename had the same shape. - Extract the preflight scan that `volume_copy` was already doing into a shared `volume_preflight.rs` module. `scan_volume_sources` reuses the cached `TransferDialog` preview when one's available (the common dialog-driven path is hit-the-cache for free), otherwise dispatches `volume.scan_for_copy_batch`. Returns `(total_files, total_bytes, source_hints)`; also emits the `Scanning`-phase event and emits `write-cancelled` on pre-scan cancel so the FE dialog closes cleanly. - Both `move_volumes_with_progress` and `move_within_same_volume_with_progress` now call it. Real `total_bytes` flows into the driver and every per-source emit; the file-only bulk-skip uses `preflight.known_directory_paths()`. - Side cleanups the shared scan enables: * dropped `collect_known_directory_paths` (per-source stat to filter directories — replaced by `is_directory` from the cached scan). * dropped the cross-volume copy+delete loop's per-source `is_directory` probe. * dropped the same-volume rename's per-iter `get_metadata` for size. * conflict resolution now gets a real `source_size_hint`, so MTP conflict dialogs no longer re-list the parent dir per conflict. - `volume_copy` slimmed by ~104 lines; its inline scan/cached-preview block + inline `known_directory_paths` builder are gone. - Tests: `cross_volume_move_emits_bytes_total_on_progress` + `same_volume_move_emits_bytes_total_on_progress` pin the regression.
1 parent 31d97e0 commit 8856e01

6 files changed

Lines changed: 484 additions & 238 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ Pre-flight scans reuse cached listings when the source volume reports an active
3333
| `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. |
3434
| `volume_copy.rs` | Volume-to-volume copy (Local↔MTP↔SMB): `copy_between_volumes`, `scan_for_volume_copy`. Uses `OperationEventSink` (not `AppHandle` directly) for event emission. Handles conflict detection, resolution, progress, rollback (delete all copied files in reverse with progress), and partial-file cleanup on cancel. Shared `map_volume_error` helper. |
3535
| `volume_move.rs` | Volume-to-volume move: `move_between_volumes`, `move_within_same_volume`. Same-volume uses `Volume::rename`; cross-volume does copy+delete. |
36+
| `volume_preflight.rs` | Shared preflight scan for both volume copy and move: `scan_volume_sources` returns a `VolumePreflight { total_files, total_bytes, source_hints }`. Reuses a cached preview from `TransferDialog` when one is available; otherwise dispatches `volume.scan_for_copy_batch` (so MTP's group-by-parent and SMB's pipelined-stat optimizations still kick in). Emits one `WriteProgressEvent { phase: Scanning, … }` so the FE sees the scan stage. On pre-scan cancel, emits `write-cancelled` and returns `Err(Cancelled)` so the FE dialog closes cleanly. |
3637
| `volume_conflict.rs`, `volume_strategy.rs` | Conflict resolution (Stop/Skip/Overwrite/Rename/OverwriteSmaller/OverwriteOlder) and copy strategy selection for volume operations. `volume_conflict.rs` mirrors the local-FS `reduce_conditional_resolution` from `helpers.rs` with its own `reduce_volume_conditional_resolution` (async, uses size hints + `get_metadata` for mtime). |
3738
| `transfer_driver.rs` | Shared per-source transfer driver for copy/move ops. Owns the bulk-skip prelude, per-iter cancellation check, conflict-resolve dispatch (async path), per-iter skip accounting, and paired progress/status emit. Two sibling entry points: `drive_transfer_serial_sync` (used by `copy.rs::copy_files_with_progress_inner`; closure captures `&mut CopyTransaction` / `&mut created_dirs` / `&mut SourceItemTracker`) and `drive_transfer_serial_async` (used by `volume_copy::copy_volumes_with_progress`'s serial path and both `volume_move` paths; driver owns top-level conflict detection + dispatch). The `FuturesUnordered` concurrent path in `volume_copy.rs` stays inline (1-of-4 abstraction; see plan § "Concurrent driver scope"). |
3839
| `tests.rs` | Unit tests. |
@@ -276,6 +277,9 @@ volume-delete handler.
276277
**Decision**: Volume delete reuses the scan preview and is oracle-aware on the no-preview path.
277278
**Why**: Before this, `delete_volume_files_with_progress_inner` ignored `config.preview_id` entirely and ran `scan_volume_recursive` again. On MTP that meant a second 17 s parent listing for a 135-photo `/DCIM/Camera` delete after the user had just paid that cost in the pre-flight dialog — and the second scan emitted no per-top-level-file progress, so the UI looked frozen. The fix has three parts. (1) `delete_volume_files_with_progress_inner` calls `take_cached_scan_result(preview_id)` at the top; on hit, top-level files are recorded from `CopyScanResult::total_bytes` with no `is_directory` probe and no `list_directory` round-trip, and top-level dirs recurse via the oracle-aware `scan_volume_recursive` (passing `is_dir_hint = Some(true)` so the recursion never re-probes). (2) The walker's internal `volume.list_directory(path, ...)` is now preceded by `try_get_watched_listing(volume_id, path)`; on hit, the cached entries replace the volume call entirely at every recursion level. (3) On the no-preview path (MCP triggers, programmatic deletes), the top-level `volume.is_directory(source)` probe stays only when the parent oracle misses — when a pane has the source's parent open and watcher-fresh, the type comes from the cached `FileEntry` and the probe is skipped. The cache-hit path also emits a throttled scan-progress event per `progress_interval` while building the entry list, so the FE dialog shows movement during the fast path instead of waiting for the delete phase to start. Pinned by `delete_volume_reuse_tests.rs`. Data-safety contract: stale-by-one cached entries can either silently skip a now-gone file (acceptable: the user already moved it elsewhere) or attempt to delete a missing one (the volume's `delete` errors cleanly). Neither direction can delete the wrong file because we feed `volume.delete(&entry.path)` exact paths the cache observed; a cached entry that races with a concurrent rename ends up addressing the old path the next call won't find.
278279

280+
**Decision**: Volume move runs the same preflight scan as volume copy (extracted to `volume_preflight.rs`).
281+
**Why**: Both `move_volumes_with_progress` (cross-volume copy+delete) and `move_within_same_volume_with_progress` (rename) used to skip the scan phase entirely, sending `bytes_total = 0` on every progress event. The FE's `TransferProgressDialog` hides the Size progress bar behind `{#if bytesTotal > 0}`, so during an MTP→local move the user saw only the Files bar with no size feedback — even though `bytes_done` was being tracked correctly. The fix shares one helper (`scan_volume_sources`) between copy and move: reuses a cached `TransferDialog` preview when available (free in the common dialog-driven path), falls through to `volume.scan_for_copy_batch` otherwise. Move now gets the same `(total_files, total_bytes, source_hints)` triple copy has, so progress events carry the real `bytes_total`, the per-source `is_directory` probe inside the move loop is gone (hint comes from the scan), and the same-volume rename's per-iter `get_metadata` for size is gone (hint again). The previous `collect_known_directory_paths` helper (file-only-bulk-skip via per-source `get_metadata`) is replaced by `VolumePreflight::known_directory_paths()`. Behavior change to flag: programmatic moves with no `preview_id` (MCP, etc.) now pay one batch scan up front; for MTP this is ~one parent listing's RTT. Same cost copy has always paid; consistent across both ops. Pinned by `volume_move::tests::*_emits_bytes_total_on_progress`.
282+
279283
**Decision**: Keep `exacl` crate for ACL copy in chunked copies (not custom FFI bindings).
280284
**Why**: `exacl` adds zero new transitive dependencies (all of its deps, `bitflags`, `log`, `scopeguard`, `uuid`, are already in our tree). It provides cross-platform ACL support (macOS, Linux, FreeBSD) and full ACL parsing/manipulation for potential future UI features. The crate appears unmaintained (last release Feb 2024) but ACL APIs are stable and don't change. Our usage is best-effort with graceful fallback: if `exacl` ever breaks, files still copy, they just lose ACLs. MIT licensed (compatible with BSL).
281285

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ mod types;
3636
mod volume_conflict;
3737
mod volume_copy;
3838
mod volume_move;
39+
mod volume_preflight;
3940
mod volume_strategy;
4041

4142
use std::path::PathBuf;

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

Lines changed: 26 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -28,31 +28,21 @@ use uuid::Uuid;
2828
use futures_util::StreamExt;
2929
use futures_util::stream::FuturesUnordered;
3030

31-
use super::scan::take_cached_scan_result;
32-
use super::transfer_driver::{
33-
ConflictDecision, ConflictDecisionInput, DriverConfig, PostLoopIntent, TransferContext, TransferOutcome,
34-
build_pre_skip_set, drive_transfer_serial_async,
35-
};
36-
37-
/// Per-source hints collected during the scan phase, so the copy loop can
38-
/// skip re-probing the source type/size per file. `size` is only meaningful
39-
/// when `is_directory == false`; it's the top-level file's size and feeds
40-
/// the SMB compound fast-path.
41-
#[derive(Clone, Copy, Default)]
42-
struct SourceHint {
43-
is_directory: bool,
44-
size: u64,
45-
}
4631
use super::state::{
4732
OperationIntent, WRITE_OPERATION_STATE, WriteOperationState, is_cancelled, load_intent, register_operation_status,
4833
unregister_operation_status, update_operation_status,
4934
};
35+
use super::transfer_driver::{
36+
ConflictDecision, ConflictDecisionInput, DriverConfig, PostLoopIntent, TransferContext, TransferOutcome,
37+
build_pre_skip_set, drive_transfer_serial_async,
38+
};
5039
use super::types::{
5140
ConflictResolution, OperationEventSink, TauriEventSink, VolumeCopyConfig, VolumeCopyScanResult,
5241
WriteCancelledEvent, WriteCompleteEvent, WriteErrorEvent, WriteOperationConfig, WriteOperationError,
5342
WriteOperationPhase, WriteOperationStartResult, WriteOperationType, WriteProgressEvent,
5443
};
5544
use super::volume_conflict::resolve_volume_conflict;
45+
use super::volume_preflight::{SourceHint, scan_volume_sources};
5646
use super::volume_strategy::copy_single_path;
5747
use crate::file_system::volume::{SourceItemInfo, Volume, VolumeError};
5848

@@ -422,112 +412,24 @@ pub(crate) async fn copy_volumes_with_progress(
422412
source_paths.len()
423413
);
424414

425-
// Phase 1: Scan sources (or reuse cached scan from preview)
426-
let total_files;
427-
let total_bytes;
428-
429-
// Per-source hint collected during the scan: whether the top-level path
430-
// is a directory and, for top-level files, the file size. The copy loop
431-
// reuses these to skip an `is_directory` probe per file and, for SMB, to
432-
// pick the 1-RTT compound fast-path when the file fits in one READ.
433-
let mut source_hints: HashMap<PathBuf, SourceHint> = HashMap::with_capacity(source_paths.len());
434-
435-
if let Some(cached) = config.preview_id.as_deref().and_then(take_cached_scan_result) {
436-
total_files = cached.file_count;
437-
total_bytes = cached.total_bytes;
438-
log::debug!(
439-
"copy_volumes_with_progress: reused cached scan for operation_id={}, files={}, bytes={}, per_path={}",
440-
operation_id,
441-
total_files,
442-
total_bytes,
443-
cached.per_path.len()
444-
);
445-
// Volume scan previews carry per-path results so we can seed source_hints
446-
// directly. Without this, we'd `is_directory` each path here, and on MTP
447-
// every `is_directory` lists the parent dir (15k photos in /DCIM/Camera =
448-
// 15k sequential parent listings, ~2 min stall before the copy starts).
449-
// Local-FS scans don't populate per_path; the unwrap_or default leaves
450-
// source_hints empty (the conflict-resolution and SMB compound fast-paths
451-
// both fall back cleanly when a hint is missing).
452-
for (source_path, scan) in cached.per_path {
453-
let size = if scan.top_level_is_directory {
454-
0
455-
} else {
456-
scan.total_bytes
457-
};
458-
source_hints.insert(
459-
source_path,
460-
SourceHint {
461-
is_directory: scan.top_level_is_directory,
462-
size,
463-
},
464-
);
465-
}
466-
} else {
467-
log::debug!(
468-
"copy_volumes_with_progress: scanning sources for operation_id={}",
469-
operation_id
470-
);
471-
472-
state.emit_progress_via_sink(
473-
&*events,
474-
WriteProgressEvent::new(
475-
operation_id.to_string(),
476-
WriteOperationType::Copy,
477-
WriteOperationPhase::Scanning,
478-
None,
479-
0,
480-
0,
481-
0,
482-
0,
483-
),
484-
);
485-
486-
if is_cancelled(&state.intent) {
487-
return Err(WriteFailure::synthetic(WriteOperationError::Cancelled {
488-
message: "Operation cancelled by user".to_string(),
489-
}));
490-
}
491-
492-
// Single pipelined batch scan. For SMB this fires N stat requests
493-
// over one session in parallel instead of N sequential RTTs (Fix 4).
494-
// Default impl loops per-path for backends where per-path I/O is
495-
// cheap (local FS, in-memory). MTP overrides to group by parent dir.
496-
let batch = source_volume.scan_for_copy_batch(source_paths).await.map_err(|e| {
497-
// No specific source path here; pick the first one or fall back to the dest.
498-
let path = source_paths.first().cloned().unwrap_or_else(|| dest_path.to_path_buf());
499-
WriteFailure::from_volume(&path, e)
500-
})?;
501-
502-
total_files = batch.aggregate.file_count;
503-
let total_dirs = batch.aggregate.dir_count;
504-
total_bytes = batch.aggregate.total_bytes;
505-
506-
for (source_path, scan) in &batch.per_path {
507-
// For top-level files, `scan.total_bytes` == the file size.
508-
// For directories, we leave `size = 0` (unused downstream).
509-
let size = if scan.top_level_is_directory {
510-
0
511-
} else {
512-
scan.total_bytes
513-
};
514-
source_hints.insert(
515-
source_path.clone(),
516-
SourceHint {
517-
is_directory: scan.top_level_is_directory,
518-
size,
519-
},
520-
);
521-
}
522-
523-
log::debug!(
524-
"copy_volumes_with_progress: scan complete for operation_id={}, files={}, dirs={}, bytes={}",
525-
operation_id,
526-
total_files,
527-
total_dirs,
528-
total_bytes
529-
);
530-
}
415+
// Phase 1: Preflight scan (reuses the dialog's cached preview when one is
416+
// available). Populates `total_files`, `total_bytes`, and per-source
417+
// `is_directory` / `size` hints so the copy loop doesn't have to re-probe
418+
// each source. Shared with the move pipeline.
419+
let preflight = scan_volume_sources(
420+
&source_volume,
421+
source_paths,
422+
config,
423+
operation_id,
424+
WriteOperationType::Copy,
425+
state,
426+
&*events,
427+
)
428+
.await?;
429+
let total_files = preflight.total_files;
430+
let total_bytes = preflight.total_bytes;
431+
let known_directory_paths = preflight.known_directory_paths();
432+
let mut source_hints = preflight.source_hints;
531433

532434
// Phase 2: Check destination space
533435
let dest_space = dest_volume
@@ -613,15 +515,9 @@ pub(crate) async fn copy_volumes_with_progress(
613515
// conflict instead of jumping to the full skipped count immediately.
614516
// Bulk-skip is **file-only**: a top-level directory's name matching a
615517
// pre-known conflict means only some of its children collide at dest, so
616-
// dropping the whole subtree would lose non-conflicting files. We collect
617-
// the top-level directory paths from `source_hints` (populated by the
618-
// batched scan above) and exclude them; the loop falls through to
619-
// per-iter conflict resolution for those.
620-
let known_directory_paths: HashSet<PathBuf> = source_hints
621-
.iter()
622-
.filter(|&(_path, hint)| hint.is_directory)
623-
.map(|(path, _hint)| path.clone())
624-
.collect();
518+
// dropping the whole subtree would lose non-conflicting files. Top-level
519+
// directory paths come from `preflight.known_directory_paths()` (computed
520+
// from the batched scan's `is_directory` hints).
625521
let pre_skip_paths: HashSet<PathBuf> = build_pre_skip_set(
626522
source_paths,
627523
config.conflict_resolution,

0 commit comments

Comments
 (0)