fix: handle deleting files from smb shares#284
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances file deletion capabilities by introducing a more robust and provider-agnostic approach. It centralizes deletion logic through a new provider layer, allowing for differentiated handling of various file systems like local, SMB, and Google Drive. This change improves reliability for network shares by implementing recursive deletion and intelligent fallback strategies, while also refining UI feedback for trash operations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the file deletion logic to support various providers like local filesystem, Google Drive, and SMB shares. The introduction of TrashStrategy and the provider-based approach in trash_paths and delete_paths_permanently are excellent architectural improvements. The backend changes are correctly reflected in the frontend, including the conditional display of the 'Restore via Finder' message. I have a couple of suggestions to make the Rust code in src-tauri/src/commands.rs more idiomatic, but overall, this is a solid contribution.
| let mut targets = Vec::with_capacity(paths.len()); | ||
|
|
||
| for original in paths { | ||
| let (provider, location) = resolve_location(LocationInput::Raw(original))?; | ||
| let capabilities = provider.capabilities(&location); | ||
| if !capabilities.can_delete { | ||
| return Err("Provider does not support deleting items".to_string()); | ||
| } | ||
|
|
||
| let display_path = if location.scheme() == "file" { | ||
| expand_path(&location.to_path_string())? | ||
| .to_string_lossy() | ||
| .to_string() | ||
| } else { | ||
| location.raw().to_string() | ||
| }; | ||
|
|
||
| targets.push(DeleteTarget { | ||
| provider, | ||
| location, | ||
| display_path, | ||
| }); | ||
| } | ||
|
|
||
| Ok(targets) |
There was a problem hiding this comment.
This function can be written more idiomatically using Rust's iterator methods, which can make the code more concise and expressive. By using into_iter(), map(), and collect(), you can transform the vector of paths into a Result containing a vector of DeleteTargets, while still handling errors gracefully as the collect() over an iterator of Results will short-circuit on the first Err.
paths
.into_iter()
.map(|original| {
let (provider, location) = resolve_location(LocationInput::Raw(original))?;
let capabilities = provider.capabilities(&location);
if !capabilities.can_delete {
return Err("Provider does not support deleting items".to_string());
}
let display_path = if location.scheme() == "file" {
expand_path(&location.to_path_string())?
.to_string_lossy()
.to_string()
} else {
location.raw().to_string()
};
Ok(DeleteTarget {
provider,
location,
display_path,
})
})
.collect()| if targets | ||
| .iter() | ||
| .any(|target| trash_strategy_for_scheme(target.location.scheme()) != strategy) | ||
| { |
There was a problem hiding this comment.
The current check iterates over all targets to ensure they use the same trash strategy. Since the strategy is determined from the first target, you can make this slightly more efficient by skipping the first element in the iterator. This avoids an unnecessary check for the first element against itself.
| if targets | |
| .iter() | |
| .any(|target| trash_strategy_for_scheme(target.location.scheme()) != strategy) | |
| { | |
| if targets | |
| .iter() | |
| .skip(1) | |
| .any(|target| trash_strategy_for_scheme(target.location.scheme()) != strategy) | |
| { |
There was a problem hiding this comment.
Code Review
This pull request is a great architectural improvement, introducing a provider layer for deletion operations to correctly handle file deletions from SMB shares, including recursive directory deletion. The changes are well-structured, and the fallback from trash to permanent delete for providers that don't support it is a thoughtful UX decision. I have one suggestion to improve the performance of permanent deletion for multiple files.
| for (idx, target) in targets.iter().enumerate() { | ||
| let current_path = target.display_path.clone(); | ||
| emit_delete_progress_update( | ||
| &app, | ||
| DeleteProgressUpdatePayload { | ||
| request_id: request_id.clone(), | ||
| current_path: Some(current_path.clone()), | ||
| completed: idx, | ||
| total, | ||
| finished: false, | ||
| error: None, | ||
| }, | ||
| ); | ||
|
|
||
| tauri::async_runtime::spawn_blocking(move || -> Result<(), String> { | ||
| for (idx, target) in targets.iter().enumerate() { | ||
| let current_path = target.to_string_lossy().to_string(); | ||
| if let Err(err) = target.provider.delete(&target.location).await { | ||
| emit_delete_progress_update( | ||
| &app_handle, | ||
| &app, | ||
| DeleteProgressUpdatePayload { | ||
| request_id: request_id_clone.clone(), | ||
| current_path: Some(current_path.clone()), | ||
| request_id: request_id.clone(), | ||
| current_path: Some(current_path), | ||
| completed: idx, | ||
| total, | ||
| finished: false, | ||
| error: None, | ||
| finished: true, | ||
| error: Some(err.clone()), | ||
| }, | ||
| ); | ||
|
|
||
| if let Err(err) = delete_file_or_directory(target) { | ||
| emit_delete_progress_update( | ||
| &app_handle, | ||
| DeleteProgressUpdatePayload { | ||
| request_id: request_id_clone.clone(), | ||
| current_path: Some(current_path), | ||
| completed: idx, | ||
| total, | ||
| finished: true, | ||
| error: Some(err.clone()), | ||
| }, | ||
| ); | ||
| return Err(err); | ||
| } | ||
| return Err(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for delete_paths_permanently deletes files sequentially. This can be slow when deleting multiple files from a remote source like SMB or GDrive. You could improve performance by parallelizing the delete operations.
Using futures::stream::try_for_each_concurrent would allow you to process multiple deletions in parallel while still stopping on the first error, preserving the current error handling behavior.
You'll need to add use futures::stream::{self, TryStreamExt}; at the top of the file.
let result = stream::iter(targets.into_iter().enumerate())
.try_for_each_concurrent(4, |(idx, target)| async {
let current_path = target.display_path.clone();
emit_delete_progress_update(
&app,
DeleteProgressUpdatePayload {
request_id: request_id.clone(),
current_path: Some(current_path.clone()),
completed: idx,
total,
finished: false,
error: None,
},
);
target.provider.delete(&target.location).await.map_err(|err| {
emit_delete_progress_update(
&app,
DeleteProgressUpdatePayload {
request_id: request_id.clone(),
current_path: Some(current_path),
completed: idx,
total,
finished: true,
error: Some(err.clone()),
},
);
err
})
})
.await;
if let Err(err) = result {
return Err(err);
}
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement, refactoring the file deletion logic to support various providers like local filesystem, SMB, and Google Drive. The introduction of TrashStrategy is a clean way to handle different deletion behaviors. The recursive deletion for SMB directories and the UI enhancements for clipboard shortcuts and trash notifications are well-executed. However, I've identified a critical performance regression in the permanent deletion logic for local files. A blocking I/O operation is now performed on the async runtime, which could freeze the UI. My review includes a detailed comment and a suggested fix for this issue.
| if let Err(err) = target.provider.delete(&target.location).await { | ||
| emit_delete_progress_update( | ||
| &app_handle, | ||
| &app, | ||
| DeleteProgressUpdatePayload { | ||
| request_id: request_id_clone.clone(), | ||
| current_path: Some(current_path.clone()), | ||
| request_id: request_id.clone(), | ||
| current_path: Some(current_path), | ||
| completed: idx, | ||
| total, | ||
| finished: false, | ||
| error: None, | ||
| finished: true, | ||
| error: Some(err.clone()), | ||
| }, | ||
| ); | ||
|
|
||
| if let Err(err) = delete_file_or_directory(target) { | ||
| emit_delete_progress_update( | ||
| &app_handle, | ||
| DeleteProgressUpdatePayload { | ||
| request_id: request_id_clone.clone(), | ||
| current_path: Some(current_path), | ||
| completed: idx, | ||
| total, | ||
| finished: true, | ||
| error: Some(err.clone()), | ||
| }, | ||
| ); | ||
| return Err(err); | ||
| } | ||
| return Err(err); | ||
| } |
There was a problem hiding this comment.
The refactoring of delete_paths_permanently removed the tauri::async_runtime::spawn_blocking call. The new implementation awaits target.provider.delete() directly in an async loop.
This is a critical issue because for the local filesystem provider, the delete method performs blocking I/O. Executing this on the async runtime will freeze the UI when deleting large directories.
To fix this, blocking operations for the file scheme should be moved to a blocking thread. You can achieve this by wrapping the delete future in spawn_blocking and using futures::executor::block_on to run it to completion on the blocking thread.
Note: You may need to add use futures::executor; at the top of the file.
let delete_result = if target.location.scheme() == "file" {
let provider = Arc::clone(&target.provider);
let location = target.location.clone();
tauri::async_runtime::spawn_blocking(move || {
futures::executor::block_on(provider.delete(&location))
})
.await
.map_err(|e| e.to_string())?
} else {
target.provider.delete(&target.location).await
};
if let Err(err) = delete_result {
emit_delete_progress_update(
&app,
DeleteProgressUpdatePayload {
request_id: request_id.clone(),
current_path: Some(current_path),
completed: idx,
total,
finished: true,
error: Some(err.clone()),
},
);
return Err(err);
}
There was a problem hiding this comment.
Code Review
This pull request refactors file deletion to support different providers like SMB, which is a great improvement. The core logic is moved to a provider layer, and recursive deletion for SMB directories is added. The changes look solid. I've added a few suggestions: one to fix a potential path joining bug in the SMB implementation, another to improve performance by parallelizing delete operations for providers that support it, and a third to refactor a function for better readability and more helpful error messages.
| fn join_smb_path(parent: &str, name: &str) -> String { | ||
| let trimmed = parent.trim_end_matches('/'); | ||
| if trimmed.is_empty() { | ||
| format!("/{}", name) | ||
| } else { | ||
| format!("{}/{}", trimmed, name) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of join_smb_path incorrectly handles an empty parent path. If parent is an empty string, it will produce /{name} which is likely an incorrect path. An empty parent path should result in just {name}. This can be fixed by handling the empty parent path case correctly. I've also added an explicit check for the root path / for clarity.
fn join_smb_path(parent: &str, name: &str) -> String {
if parent == "/" {
return format!("/{}", name);
}
let trimmed = parent.trim_end_matches('/');
if trimmed.is_empty() {
name.to_string()
} else {
format!("{}/{}", trimmed, name)
}
}| let mut targets = Vec::with_capacity(paths.len()); | ||
|
|
||
| for original in paths { | ||
| let (provider, location) = resolve_location(LocationInput::Raw(original))?; | ||
| let capabilities = provider.capabilities(&location); | ||
| if !capabilities.can_delete { | ||
| return Err("Provider does not support deleting items".to_string()); | ||
| } | ||
|
|
||
| let display_path = if location.scheme() == "file" { | ||
| expand_path(&location.to_path_string())? | ||
| .to_string_lossy() | ||
| .to_string() | ||
| } else { | ||
| location.raw().to_string() | ||
| }; | ||
|
|
||
| targets.push(DeleteTarget { | ||
| provider, | ||
| location, | ||
| display_path, | ||
| }); | ||
| } | ||
|
|
||
| Ok(targets) |
There was a problem hiding this comment.
This function can be refactored to use a more idiomatic iterator-based approach with map and collect. This makes the code more concise. I've also taken the opportunity to improve the error message to include the path that caused the issue, which is helpful for debugging.
paths
.into_iter()
.map(|original| {
let (provider, location) = resolve_location(LocationInput::Raw(original))?;
let capabilities = provider.capabilities(&location);
if !capabilities.can_delete {
return Err(format!(
"Provider for path '{}' does not support deleting items",
location.raw()
));
}
let display_path = if location.scheme() == "file" {
expand_path(&location.to_path_string())?
.to_string_lossy()
.to_string()
} else {
location.raw().to_string()
};
Ok(DeleteTarget {
provider,
location,
display_path,
})
})
.collect()| for target in &targets { | ||
| target.provider.delete(&target.location).await?; | ||
| } |
There was a problem hiding this comment.
The current implementation deletes files from the provider sequentially. When deleting multiple files from a remote provider (like Google Drive), this can be slow. You can improve performance by running the delete operations in parallel using futures::future::try_join_all. This will execute all delete futures concurrently and fail fast if any of them error, preserving the current error handling logic.
| for target in &targets { | |
| target.provider.delete(&target.location).await?; | |
| } | |
| let delete_futures = targets.iter().map(|target| target.provider.delete(&target.location)); | |
| futures::future::try_join_all(delete_futures).await?; |
1 participant
Summary
Verification