Skip to content

fix: handle deleting files from smb shares #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BrianLeishman wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: handle deleting files from smb shares #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09c616d
fix: handle deleting files from smb shares
BrianLeishman Mar 12, 2026
5bc28d7
fix: require modifier for file clipboard shortcuts
BrianLeishman Mar 12, 2026
7a7fc00
chore: format app clipboard shortcut fix
BrianLeishman Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 129 additions & 62 deletions src-tauri/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,55 @@ fn normalize_path_for_compare(path: &Path) -> String {
value
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum TrashStrategy {
SystemTrash,
ProviderTrash,
PermanentDeleteFallback,
}

fn trash_strategy_for_scheme(scheme: &str) -> TrashStrategy {
match scheme {
"file" => TrashStrategy::SystemTrash,
"gdrive" => TrashStrategy::ProviderTrash,
_ => TrashStrategy::PermanentDeleteFallback,
}
}

struct DeleteTarget {
provider: crate::locations::ProviderRef,
location: Location,
display_path: String,
}

fn resolve_delete_targets(paths: Vec<String>) -> Result<Vec<DeleteTarget>, String> {
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)
Comment on lines +291 to +315
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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()

Comment on lines +291 to +315
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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()

}

fn cleanup_trash_undo_records(state: &TrashUndoState) {
const TTL: Duration = Duration::from_secs(300);
const MAX_RECORDS: usize = 10;
Expand Down Expand Up @@ -714,6 +763,7 @@ pub struct TrashPathsResponse {
trashed: Vec<String>,
undo_token: Option<String>,
fallback_to_permanent: bool,
used_system_trash: bool,
}

#[derive(Debug, Serialize, Clone)]
Expand Down Expand Up @@ -1974,22 +2024,55 @@ pub async fn trash_paths(app: AppHandle, paths: Vec<String>) -> Result<TrashPath
trashed: Vec::new(),
undo_token: None,
fallback_to_permanent: false,
used_system_trash: false,
});
}

let mut expanded: Vec<PathBuf> = Vec::with_capacity(paths.len());
for original in paths {
let path_buf = expand_path(&original)?;
if !path_buf.exists() {
return Err(format!("Path does not exist: {}", original));
let targets = resolve_delete_targets(paths)?;
let strategy = trash_strategy_for_scheme(targets[0].location.scheme());
if targets
.iter()
.any(|target| trash_strategy_for_scheme(target.location.scheme()) != strategy)
{
Comment on lines +2033 to +2036
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)
{

return Err("Delete selection must come from a single provider type".to_string());
}

let original_paths: Vec<String> = targets
.iter()
.map(|target| target.display_path.clone())
.collect();

if strategy == TrashStrategy::PermanentDeleteFallback {
return Ok(TrashPathsResponse {
trashed: Vec::new(),
undo_token: None,
fallback_to_permanent: true,
used_system_trash: false,
});
}

if strategy == TrashStrategy::ProviderTrash {
for target in &targets {
target.provider.delete(&target.location).await?;
}
Comment on lines +2055 to 2057
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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?;

expanded.push(path_buf);

return Ok(TrashPathsResponse {
trashed: original_paths,
undo_token: None,
fallback_to_permanent: false,
used_system_trash: false,
});
}

let original_paths: Vec<String> = expanded
let expanded: Vec<PathBuf> = targets
.iter()
.map(|path| path.to_string_lossy().to_string())
.map(|target| PathBuf::from(&target.display_path))
.collect();
for path_buf in &expanded {
if !path_buf.exists() {
return Err(format!("Path does not exist: {}", path_buf.to_string_lossy()));
}
}

let delete_targets: Vec<PathBuf> = expanded.clone();
let state = app.state::<TrashUndoState>();
Expand Down Expand Up @@ -2018,6 +2101,7 @@ pub async fn trash_paths(app: AppHandle, paths: Vec<String>) -> Result<TrashPath
trashed: Vec::new(),
undo_token: None,
fallback_to_permanent: true,
used_system_trash: false,
});
}
return Err(err);
Expand Down Expand Up @@ -2103,6 +2187,7 @@ pub async fn trash_paths(app: AppHandle, paths: Vec<String>) -> Result<TrashPath
trashed: original_paths,
undo_token,
fallback_to_permanent: false,
used_system_trash: true,
})
}

Expand Down Expand Up @@ -2191,72 +2276,54 @@ pub async fn delete_paths_permanently(
});
}

let mut expanded: Vec<PathBuf> = Vec::with_capacity(paths.len());
for original in paths {
let path_buf = expand_path(&original)?;
if !path_buf.exists() {
return Err(format!("Path does not exist: {}", original));
}
expanded.push(path_buf);
}

let total = expanded.len();
let original_paths: Vec<String> = expanded
let targets = resolve_delete_targets(paths)?;
let total = targets.len();
let original_paths: Vec<String> = targets
.iter()
.map(|path| path.to_string_lossy().to_string())
.map(|target| target.display_path.clone())
.collect();
let targets: Vec<PathBuf> = expanded.clone();

let app_handle = app.clone();
let request_id_clone = request_id.clone();
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);
}
Comment on lines +2300 to 2313
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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);
        }

}
Comment on lines +2286 to +2314
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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);
    }


emit_delete_progress_update(
&app_handle,
DeleteProgressUpdatePayload {
request_id: request_id_clone,
current_path: None,
completed: total,
total,
finished: true,
error: None,
},
);

Ok(())
})
.await
.map_err(|err| format!("Failed to join delete task: {err}"))??;
emit_delete_progress_update(
&app,
DeleteProgressUpdatePayload {
request_id,
current_path: None,
completed: total,
total,
finished: true,
error: None,
},
);

Ok(DeletePathsResponse {
deleted: original_paths,
Expand Down
51 changes: 45 additions & 6 deletions src-tauri/src/smb_sidecar/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,50 @@ fn is_hidden_file(name: &str) -> bool {
name.starts_with('.')
}

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)
}
}
Comment on lines +67 to +74
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)
    }
}


fn delete_directory_recursive(client: &SmbClient, path: &str) -> Result<(), (i32, String)> {
let entries = client
.list_dirplus(path)
.map_err(|e| {
let (code, msg) = map_smb_error(&e);
(code, format!("Failed to list directory for deletion: {}", msg))
})?;

for entry in entries {
let name = entry.name();
if name == "." || name == ".." {
continue;
}

let child_path = join_smb_path(path, name);
if matches!(entry.get_type(), pavao::SmbDirentType::Dir) {
delete_directory_recursive(client, &child_path)?;
} else {
client
.unlink(&child_path)
.map_err(|e| {
let (code, msg) = map_smb_error(&e);
(code, format!("Failed to delete file {}: {}", child_path, msg))
})?;
}
}

client
.rmdir(path)
.map_err(|e| {
let (code, msg) = map_smb_error(&e);
(code, format!("Failed to delete directory {}: {}", path, msg))
})
}

/// Read directory contents.
pub fn read_directory(params: ReadDirectoryParams) -> Result<ReadDirectoryResult, (i32, String)> {
let _guard = SMB_MUTEX
Expand Down Expand Up @@ -215,12 +259,7 @@ pub fn delete(params: DeleteParams) -> Result<(), (i32, String)> {
})?;

if stat.mode.is_dir() {
client
.rmdir(&params.path)
.map_err(|e| {
let (code, msg) = map_smb_error(&e);
(code, format!("Failed to delete directory: {}", msg))
})
delete_directory_recursive(&client, &params.path)
} else {
client
.unlink(&params.path)
Expand Down
8 changes: 5 additions & 3 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1784,8 +1784,10 @@ function App() {
// when NOT in an editable field to do file operations instead.
{
const keyLc = e.key.toLowerCase();
const hasClipboardModifier =
!e.altKey && !e.shiftKey && ((isMac && e.metaKey && !e.ctrlKey) || (!isMac && e.ctrlKey));

if (keyLc === 'c' && !inEditable) {
if (hasClipboardModifier && keyLc === 'c' && !inEditable) {
const state = useAppStore.getState();
if (state.selectedFiles.length > 0) {
e.preventDefault();
Expand All @@ -1794,7 +1796,7 @@ function App() {
}
}

if (keyLc === 'x' && !inEditable) {
if (hasClipboardModifier && keyLc === 'x' && !inEditable) {
const state = useAppStore.getState();
if (state.selectedFiles.length > 0) {
e.preventDefault();
Expand All @@ -1803,7 +1805,7 @@ function App() {
}
}

if (keyLc === 'v' && !inEditable) {
if (hasClipboardModifier && keyLc === 'v' && !inEditable) {
e.preventDefault();
void useAppStore.getState().pasteFiles();
return;
Expand Down
2 changes: 2 additions & 0 deletions src/__tests__/unit/store/useAppStore.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ describe('useAppStore', () => {
trashed: ['/test/file.txt'],
undoToken: 'test-token',
fallbackToPermanent: false,
usedSystemTrash: true,
});
}
if (cmd === 'read_directory') {
Expand Down Expand Up @@ -659,6 +660,7 @@ describe('useAppStore', () => {
trashed: [],
undoToken: null,
fallbackToPermanent: true,
usedSystemTrash: false,
});
}
if (cmd === 'delete_paths_permanently') {
Expand Down
4 changes: 3 additions & 1 deletion src/store/useAppStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,9 @@ export const useAppStore = create<AppState>((set, get) => ({
const undoToken = response.undoToken;
const isMacPlatform = typeof navigator !== 'undefined' && /mac/i.test(navigator.userAgent);
const infoMessage =
undoToken || !isMacPlatform ? messageText : `${messageText} Restore via Finder if needed.`;
undoToken || !isMacPlatform || !response.usedSystemTrash
? messageText
: `${messageText} Restore via Finder if needed.`;

if (undoToken) {
// Push to undo stack for Cmd+Z support
Expand Down
1 change: 1 addition & 0 deletions src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ export interface TrashPathsResponse {
trashed: string[];
undoToken?: string;
fallbackToPermanent: boolean;
usedSystemTrash: boolean;
}

export interface UndoTrashResponse {
Expand Down
Loading