Skip to content

Commit 89cd978

Browse files
committed
Transfer dialog: surface folder merges, decouple conflict check
- Folder collisions classify as a silent merge, not a conflict: the upfront `TransferDialog` shows an informational line ("N folders will merge with existing folders") instead of counting them toward the conflict radios. A type mismatch (file vs same-named folder) stays a real conflict on the red-warning path. - The cheap top-level conflict check now runs on mount in parallel with the (slow) scan preview, assigned before the auto-confirm branch so the MCP fast path dispatches with `conflictNames` populated. Conflict info appears as soon as the one dest listing returns, not after the recursive byte scan. - `scan_volume_for_conflicts` optionally takes the source volume id + source paths and resolves each item's real `is_directory` + size authoritatively on the source volume via ONE batched `scan_for_copy_batch` (O(top-level items), never a subtree walk), replacing the FE's name-only placeholders. Back-compatible when omitted; regenerated typed bindings. - Dir-dir merge names never enter the file bulk-skip set ("Skip all" no longer skips a folder wholesale); a type mismatch under "Overwrite all" shows the existing red warning so the bulk path isn't quieter than the per-file dialog. - Tests: backend batch-merge unit + the bulk-skip-excludes-merging-dir pin (seen red first), FE component tests for parallel render / merge classification / bulk-skip forwarding / cross-type warning / auto-confirm payload, a11y siblings for the new UI states, and an IPC contract test pinning the new 5-arg wire shape.
1 parent a9743ec commit 89cd978

11 files changed

Lines changed: 819 additions & 43 deletions

File tree

apps/desktop/src-tauri/src/commands/CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ immediately to business-logic modules. No significant logic lives here.
99
|------|--------|-------|
1010
| `mod.rs` | Re-exports | `mtp`, `network` gated behind `#[cfg(any(target_os = "macos", target_os = "linux"))]`; `volumes` behind `#[cfg(target_os = "macos")]`; `volumes_linux` behind `#[cfg(target_os = "linux")]` |
1111
| `util.rs` | Shared helpers | `TimedOut<T>`, `IpcError`, `blocking_with_timeout`, `blocking_with_timeout_flag`, `blocking_result_with_timeout`. See "Timeout-aware return types" below. |
12-
| `file_system/` | File listing & writes | Directory module split by operation type. `mod.rs` has `expand_tilde()`, re-exports, and tests. `listing.rs`: streaming + virtual-scroll listing API, path queries, `find_first_fuzzy_match` (type-to-jump), benchmarking, `get_brief_column_text_widths` (per-column widest-filename text widths for Brief mode; replaces the removed `get_max_filename_width`). `refresh_listing` short-circuits on watcher-backed listings (`Volume::listing_is_watched(path) == true`): the cache is already kept fresh by `notify_mutation`, so a redundant full re-read after every transfer outcome (the FE's `refreshPanesAfterTransfer`) used to wedge slow volumes (MTP 17 s + USB session collision). Logs at debug `target: "refresh_listing"` when the short-circuit fires. `write_ops.rs`: create, copy, move, delete, trash, scan preview, conflict resolution, synthetic diff helpers. `volume_copy.rs`: cross-volume copy/move/scan, `SourceItemInput`. `drag.rs`: native drag, self-drag overlay. `e2e_support.rs`: feature-gated E2E/debug commands. |
12+
| `file_system/` | File listing & writes | Directory module split by operation type. `mod.rs` has `expand_tilde()`, re-exports, and tests. `listing.rs`: streaming + virtual-scroll listing API, path queries, `find_first_fuzzy_match` (type-to-jump), benchmarking, `get_brief_column_text_widths` (per-column widest-filename text widths for Brief mode; replaces the removed `get_max_filename_width`). `refresh_listing` short-circuits on watcher-backed listings (`Volume::listing_is_watched(path) == true`): the cache is already kept fresh by `notify_mutation`, so a redundant full re-read after every transfer outcome (the FE's `refreshPanesAfterTransfer`) used to wedge slow volumes (MTP 17 s + USB session collision). Logs at debug `target: "refresh_listing"` when the short-circuit fires. `write_ops.rs`: create, copy, move, delete, trash, scan preview, conflict resolution, synthetic diff helpers. `volume_copy.rs`: cross-volume copy/move/scan, `SourceItemInput`. `scan_volume_for_conflicts` optionally takes a source volume id + source paths and resolves each item's real `is_directory` + size from the source volume via ONE batched `scan_for_copy_batch` (O(top-level items), never a subtree walk), overriding the FE's name-only placeholders so dir-vs-dir collisions classify as silent merges; back-compatible when omitted. `drag.rs`: native drag, self-drag overlay. `e2e_support.rs`: feature-gated E2E/debug commands. |
1313
| `volumes.rs` | Volume management (macOS) | `list_volumes`, `get_default_volume_id`, `get_volume_space`, `resolve_path_volume` (statfs-based, no volume enumeration) |
1414
| `volumes_linux.rs` | Volume management (Linux) | Same interface as `volumes.rs`, delegates to `volumes_linux` module |
1515
| `mtp.rs` | MTP devices | Full MTP command surface (connect, disconnect, list, download, upload, delete, rename, move, scan) |

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

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,18 +129,28 @@ pub async fn scan_volume_for_copy(
129129
}
130130

131131
/// Checks which source items already exist at the destination. Returns conflict details for UI.
132+
///
133+
/// When `source_volume_id` and `source_paths` are both provided, each item's
134+
/// `is_directory` and `size` are resolved authoritatively on the source volume
135+
/// via ONE batched stat (`scan_for_copy_batch`, strictly O(top-level items),
136+
/// never a subtree walk), overriding whatever the caller passed in `source_items`.
137+
/// This lets the dialog classify dir-vs-dir collisions as silent merges without
138+
/// the FE having to plumb per-item types. Callers that don't pass the source
139+
/// volume keep the legacy name-only behavior.
132140
#[tauri::command]
133141
#[specta::specta]
134142
pub async fn scan_volume_for_conflicts(
135143
volume_id: String,
136144
source_items: Vec<SourceItemInput>,
137145
dest_path: String,
146+
source_volume_id: Option<String>,
147+
source_paths: Option<Vec<String>>,
138148
) -> Result<Vec<ScanConflict>, IpcError> {
139149
let volume = get_volume_manager()
140150
.get(&volume_id)
141151
.ok_or_else(|| IpcError::from_err(format!("Volume '{}' not found", volume_id)))?;
142152

143-
let source_items: Vec<crate::file_system::SourceItemInfo> = source_items
153+
let mut source_items: Vec<crate::file_system::SourceItemInfo> = source_items
144154
.into_iter()
145155
.map(|item| crate::file_system::SourceItemInfo {
146156
name: item.name,
@@ -149,6 +159,26 @@ pub async fn scan_volume_for_conflicts(
149159
is_directory: item.is_directory,
150160
})
151161
.collect();
162+
163+
// Resolve real per-item types and sizes from the source volume when the
164+
// caller supplied it. One batched stat, O(top-level items).
165+
if let (Some(src_volume_id), Some(src_paths)) = (source_volume_id, source_paths)
166+
&& let Some(src_volume) = get_volume_manager().get(&src_volume_id) {
167+
let paths: Vec<PathBuf> = src_paths.iter().map(PathBuf::from).collect();
168+
match tokio::time::timeout(Duration::from_secs(30), src_volume.scan_for_copy_batch(&paths)).await {
169+
Ok(Ok(batch)) => merge_source_types_from_batch(&mut source_items, &batch),
170+
// A failed source-side stat is non-fatal: fall back to the
171+
// name-only items the caller sent. Conflict detection still
172+
// works by name; only the dir/size hints degrade.
173+
Ok(Err(e)) => {
174+
log::debug!(target: "conflict_scan", "Source batch stat failed, using name-only items: {}", e);
175+
}
176+
Err(_) => {
177+
log::debug!(target: "conflict_scan", "Source batch stat timed out, using name-only items");
178+
}
179+
}
180+
}
181+
152182
let dest_path = PathBuf::from(dest_path);
153183

154184
// Run conflict scan (now async)
@@ -161,6 +191,35 @@ pub async fn scan_volume_for_conflicts(
161191
.map_err(|e| IpcError::from_err(e.to_string()))
162192
}
163193

194+
/// Overlays authoritative `is_directory` + `size` from a source-volume batch
195+
/// stat onto the caller-supplied `source_items`, matched by base filename.
196+
///
197+
/// The match key is the path's final component, which is exactly the `name`
198+
/// the FE derives for each `SourceItemInput`. An item with no batch hit keeps
199+
/// the values the caller sent (the safe fallback). For a top-level directory
200+
/// the batch's `total_bytes` is the recursive size, which we deliberately do
201+
/// NOT copy into `size` — a directory's conflict-UI size is meaningless and the
202+
/// dir-dir case never renders a size. Only files get their real size.
203+
fn merge_source_types_from_batch(
204+
source_items: &mut [crate::file_system::SourceItemInfo],
205+
batch: &crate::file_system::BatchScanResult,
206+
) {
207+
use std::collections::HashMap;
208+
let by_name: HashMap<&str, &crate::file_system::CopyScanResult> = batch
209+
.per_path
210+
.iter()
211+
.filter_map(|(path, scan)| path.file_name().and_then(|n| n.to_str()).map(|n| (n, scan)))
212+
.collect();
213+
for item in source_items.iter_mut() {
214+
if let Some(scan) = by_name.get(item.name.as_str()) {
215+
item.is_directory = scan.top_level_is_directory;
216+
if !scan.top_level_is_directory {
217+
item.size = scan.total_bytes;
218+
}
219+
}
220+
}
221+
}
222+
164223
/// Input type for source item information (used by scan_volume_for_conflicts).
165224
#[derive(Debug, Clone, serde::Deserialize, specta::Type)]
166225
#[serde(rename_all = "camelCase")]
@@ -177,3 +236,73 @@ pub struct SourceItemInput {
177236
#[serde(default)]
178237
pub is_directory: bool,
179238
}
239+
240+
#[cfg(test)]
241+
mod tests {
242+
use super::merge_source_types_from_batch;
243+
use crate::file_system::{BatchScanResult, CopyScanResult, SourceItemInfo};
244+
use std::path::PathBuf;
245+
246+
fn scan(is_dir: bool, bytes: u64) -> CopyScanResult {
247+
CopyScanResult {
248+
file_count: if is_dir { 0 } else { 1 },
249+
dir_count: if is_dir { 1 } else { 0 },
250+
total_bytes: bytes,
251+
dedup_bytes: bytes,
252+
top_level_is_directory: is_dir,
253+
}
254+
}
255+
256+
fn item(name: &str) -> SourceItemInfo {
257+
SourceItemInfo {
258+
name: name.to_string(),
259+
size: 0,
260+
modified: None,
261+
is_directory: false,
262+
}
263+
}
264+
265+
#[test]
266+
fn overlays_real_directory_flag_onto_placeholder_items() {
267+
let mut items = vec![item("photos"), item("readme.txt")];
268+
let batch = BatchScanResult {
269+
aggregate: scan(false, 0),
270+
per_path: vec![
271+
(PathBuf::from("/src/photos"), scan(true, 999_999)),
272+
(PathBuf::from("/src/readme.txt"), scan(false, 42)),
273+
],
274+
};
275+
276+
merge_source_types_from_batch(&mut items, &batch);
277+
278+
// The directory item is now flagged as such; its recursive byte total
279+
// is deliberately NOT copied into `size` (a dir's conflict size is
280+
// meaningless).
281+
assert!(items[0].is_directory);
282+
assert_eq!(items[0].size, 0);
283+
// The file item gets its real size.
284+
assert!(!items[1].is_directory);
285+
assert_eq!(items[1].size, 42);
286+
}
287+
288+
#[test]
289+
fn keeps_caller_values_when_no_batch_hit() {
290+
let mut items = vec![SourceItemInfo {
291+
name: "ghost".to_string(),
292+
size: 7,
293+
modified: Some(123),
294+
is_directory: true,
295+
}];
296+
let batch = BatchScanResult {
297+
aggregate: scan(false, 0),
298+
per_path: vec![(PathBuf::from("/src/other"), scan(false, 1))],
299+
};
300+
301+
merge_source_types_from_batch(&mut items, &batch);
302+
303+
// No matching name → the caller's values survive untouched.
304+
assert!(items[0].is_directory);
305+
assert_eq!(items[0].size, 7);
306+
assert_eq!(items[0].modified, Some(123));
307+
}
308+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ pub use volume::SmbVolume;
5050
pub use volume::manager::VolumeManager;
5151
#[allow(unused_imports, reason = "Public API re-exports for future use")]
5252
pub use volume::{
53-
CopyScanResult, InMemoryVolume, LocalPosixVolume, MutationEvent, ScanConflict, SourceItemInfo, SpaceInfo, Volume,
54-
VolumeError,
53+
BatchScanResult, CopyScanResult, InMemoryVolume, LocalPosixVolume, MutationEvent, ScanConflict, SourceItemInfo,
54+
SpaceInfo, Volume, VolumeError,
5555
};
5656
// Watcher management - init_watcher_manager must be called from lib.rs
5757
#[cfg(feature = "playwright-e2e")]

apps/desktop/src-tauri/src/file_system/write_operations/transfer/transfer_driver_tests.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,36 @@ fn build_pre_skip_set_excludes_known_directory_paths() {
199199
);
200200
}
201201

202+
/// A dir-vs-dir collision (a source folder landing on a same-named dest folder)
203+
/// must NEVER enter the file bulk-skip set, even under `Skip all`. Folders
204+
/// always merge; "Skip all" governs the clashing FILES inside the merge, not
205+
/// the folder wholesale. The upfront FE forwards the folder's name as a
206+
/// pre-known conflict, but the preflight scan also reports it via
207+
/// `known_directory_paths`, so it falls through to per-child resolution. This
208+
/// pins that the merge-not-skip-wholesale semantics hold at the bulk-skip gate.
209+
#[test]
210+
fn build_pre_skip_set_never_bulk_skips_a_merging_directory() {
211+
let sources = paths(&["/photos", "/notes.txt"]);
212+
let mut known_dirs = HashSet::new();
213+
// `/photos` is a directory (a dir-dir merge at the destination).
214+
known_dirs.insert(PathBuf::from("/photos"));
215+
let set = build_pre_skip_set(
216+
&sources,
217+
ConflictResolution::Skip,
218+
// Both names arrive as pre-known conflicts from the FE.
219+
&["photos".into(), "notes.txt".into()],
220+
&known_dirs,
221+
);
222+
// Only the file is bulk-skipped; the merging folder is left to per-child
223+
// resolution so its non-clashing children still copy.
224+
assert_eq!(set.len(), 1);
225+
assert!(set.contains(&PathBuf::from("/notes.txt")));
226+
assert!(
227+
!set.contains(&PathBuf::from("/photos")),
228+
"a merging directory must never be bulk-skipped wholesale"
229+
);
230+
}
231+
202232
// ===========================================================================
203233
// Sync driver: data-safety
204234
// ===========================================================================

apps/desktop/src/lib/file-operations/transfer/CLAUDE.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,22 @@ for the shared state machine, ETA/throughput, and settle contract.
4141
"Ask later" since a single conflict can't be asked "for each". The conditional policies map to the typed
4242
`ConflictResolution` variants `overwrite_smaller` / `overwrite_older`. See the BE doc § "Key patterns and gotchas
4343
(shared)" for the strict-comparison / fail-closed contract.
44+
- **Folders always merge; the upfront check classifies collisions.** `checkConflicts()` runs on mount **in parallel
45+
with the scan preview** (it's one cheap dest listing, not the recursive byte scan — `conflictCheckPromise` is
46+
assigned synchronously in `onMount` BEFORE the auto-confirm branch so the MCP fast path dispatches with
47+
`conflictNames` populated). Each collision is classified by the backend-resolved `sourceIsDirectory` /
48+
`destIsDirectory` flags (the BE resolves real per-item types + sizes from the source volume via one batched stat
49+
when `checkConflicts` passes `sourceVolumeId` + `sourcePaths`):
50+
- **dir + dir** → a silent merge, NOT a conflict. Surfaced as an informational line ("N folders will merge with
51+
existing folders"); never counted in `totalConflictCount`; never forwarded as a bulk-skip name (a merging folder
52+
must not be skipped wholesale).
53+
- **file + file / cross-type (file↔folder)** → a real conflict. Counts toward `totalConflictCount` and feeds the
54+
`preKnownConflicts` bulk-skip list.
55+
- The file-policy radios show when there's a real conflict OR a folder merge — a merge can surface file clashes
56+
mid-operation the upfront (top-level-only) check can't see, and the radios pre-answer them.
57+
- **Cross-type guardrail.** When a real conflict is a type mismatch AND the user selects "Overwrite all", a red
58+
warning appears (mirrors the per-file dialog's file→folder warning): overwriting replaces items of a different
59+
type, including folder contents.
4460

4561
2. **TransferProgressDialog** (operation execution)
4662
- If `scanInProgress`, subscribes to scan preview events (`scan-preview-progress`, `scan-preview-complete`, etc.) to

apps/desktop/src/lib/file-operations/transfer/TransferDialog.a11y.test.ts

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,13 @@
1111
import { describe, it, vi } from 'vitest'
1212
import { mount, tick } from 'svelte'
1313
import TransferDialog from './TransferDialog.svelte'
14+
import type { VolumeConflictInfo } from '$lib/tauri-commands'
1415
import { expectNoA11yViolations } from '$lib/test-a11y'
1516

17+
// Mutable per-test result for the conflict scan, so the merge-info and
18+
// cross-type-warning a11y cases can drive specific collision shapes.
19+
let conflictResult: VolumeConflictInfo[] = []
20+
1621
vi.mock('$lib/tauri-commands', () => ({
1722
notifyDialogOpened: vi.fn(() => Promise.resolve()),
1823
notifyDialogClosed: vi.fn(() => Promise.resolve()),
@@ -27,7 +32,7 @@ vi.mock('$lib/tauri-commands', () => ({
2732
onScanPreviewComplete: vi.fn(() => Promise.resolve(() => {})),
2833
onScanPreviewError: vi.fn(() => Promise.resolve(() => {})),
2934
onScanPreviewCancelled: vi.fn(() => Promise.resolve(() => {})),
30-
scanVolumeForConflicts: vi.fn(() => Promise.resolve([])),
35+
scanVolumeForConflicts: vi.fn(() => Promise.resolve(conflictResult)),
3136
DEFAULT_VOLUME_ID: 'root',
3237
}))
3338

@@ -94,4 +99,93 @@ describe('TransferDialog a11y', () => {
9499
await tick()
95100
await expectNoA11yViolations(target)
96101
})
102+
103+
it('dialog with a folder-merge info line has no a11y violations', async () => {
104+
conflictResult = [
105+
{
106+
sourcePath: 'photos',
107+
destPath: 'photos',
108+
sourceSize: 0,
109+
destSize: 0,
110+
sourceModified: null,
111+
destModified: null,
112+
sourceIsDirectory: true,
113+
destIsDirectory: true,
114+
},
115+
]
116+
const target = document.createElement('div')
117+
document.body.appendChild(target)
118+
mount(TransferDialog, {
119+
target,
120+
props: {
121+
operationType: 'copy',
122+
sourcePaths: ['/Users/test/photos'],
123+
destinationPath: '/Users/test/dest',
124+
direction: 'right',
125+
currentVolumeId: 'root',
126+
fileCount: 0,
127+
folderCount: 1,
128+
sourceFolderPath: '/Users/test',
129+
sortColumn: 'name',
130+
sortOrder: 'ascending',
131+
sourceVolumeId: 'root',
132+
destVolumeId: 'root',
133+
onConfirm: () => {},
134+
onCancel: () => {},
135+
},
136+
})
137+
// Let the parallel conflict check resolve so the merge line renders.
138+
for (let i = 0; i < 6; i++) {
139+
await new Promise<void>((resolve) => setTimeout(resolve, 0))
140+
await tick()
141+
}
142+
await expectNoA11yViolations(target)
143+
conflictResult = []
144+
})
145+
146+
it('dialog with the cross-type Overwrite-all warning has no a11y violations', async () => {
147+
conflictResult = [
148+
{
149+
sourcePath: 'photos',
150+
destPath: 'photos',
151+
sourceSize: 0,
152+
destSize: 0,
153+
sourceModified: null,
154+
destModified: null,
155+
sourceIsDirectory: false,
156+
destIsDirectory: true,
157+
},
158+
]
159+
const target = document.createElement('div')
160+
document.body.appendChild(target)
161+
mount(TransferDialog, {
162+
target,
163+
props: {
164+
operationType: 'copy',
165+
sourcePaths: ['/Users/test/photos'],
166+
destinationPath: '/Users/test/dest',
167+
direction: 'right',
168+
currentVolumeId: 'root',
169+
fileCount: 1,
170+
folderCount: 0,
171+
sourceFolderPath: '/Users/test',
172+
sortColumn: 'name',
173+
sortOrder: 'ascending',
174+
sourceVolumeId: 'root',
175+
destVolumeId: 'root',
176+
onConfirm: () => {},
177+
onCancel: () => {},
178+
},
179+
})
180+
for (let i = 0; i < 6; i++) {
181+
await new Promise<void>((resolve) => setTimeout(resolve, 0))
182+
await tick()
183+
}
184+
// Select Overwrite all to surface the red warning, then assert clean a11y.
185+
const overwrite = target.querySelector<HTMLInputElement>('input[type="radio"][value="overwrite"]')
186+
overwrite?.click()
187+
await tick()
188+
await expectNoA11yViolations(target)
189+
conflictResult = []
190+
})
97191
})

0 commit comments

Comments
 (0)