From 225ae7e07113680de61826e4ac7f789b3fad8485 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rossi?= Date: Wed, 30 Sep 2026 17:05:28 +0200 Subject: [PATCH 01/44] feat(sftp): drag files between panes and show file times Drag a file (or every ticked file) from one pane to the other to upload or download it, onto a folder row to land in that folder, or drop files from the OS file manager onto the remote pane. Pane to pane uses pointer events, since Tauri's native drop handler blocks HTML5 drag and drop in WebView2; OS drops come through onDragDropEvent and a new stat_local_paths command that tells files from folders. FileEntry now carries modified and created times. The GUI panes show a Modified column, plus Created on the local side (SFTP v3 has no creation time), and the TUI file manager shows the modification time when the panel is wide enough. --- crates/omnyssh-core/src/ssh/sftp.rs | 144 +++++++++-- crates/omnyssh-gui/src/bridge.rs | 2 + crates/omnyssh-gui/src/commands/sftp.rs | 15 +- crates/omnyssh-gui/src/dto.rs | 19 +- crates/omnyssh-gui/src/main.rs | 3 +- crates/omnyssh-gui/ui/e2e/sftp.spec.ts | 141 ++++++++++- crates/omnyssh-gui/ui/src/lib/bindings.ts | 11 +- crates/omnyssh-gui/ui/src/lib/ipc/commands.ts | 5 + .../ui/src/lib/ipc/subscribe.test.ts | 11 +- .../ui/src/lib/screens/SftpPane.svelte | 63 ++++- .../ui/src/lib/screens/SftpView.svelte | 225 ++++++++++++++++-- .../ui/src/lib/stores/sftp.test.ts | 40 +++- crates/omnyssh-gui/ui/src/lib/stores/sftp.ts | 30 +++ crates/omnyssh/src/app/file_manager.rs | 2 + crates/omnyssh/src/ui/file_manager.rs | 61 ++++- 15 files changed, 714 insertions(+), 58 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index f92f5468..aef3cb42 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -36,6 +36,50 @@ pub struct FileEntry { pub size: u64, /// `true` when this entry is a directory. pub is_dir: bool, + /// Last modification time, in seconds since the Unix epoch, when known. + pub modified: Option, + /// Creation time, in seconds since the Unix epoch. Local entries only, and only + /// where the file system records it: SFTP v3 has no creation time. + pub created: Option, +} + +impl FileEntry { + /// The synthetic `..` entry pointing at `parent`. + fn parent(parent: &str) -> Self { + Self { + name: "..".to_string(), + path: parent.to_string(), + size: 0, + is_dir: true, + modified: None, + created: None, + } + } +} + +/// Seconds since the Unix epoch for a file time, or `None` if it is unavailable. +fn unix_secs(time: std::io::Result) -> Option { + let time = time.ok()?; + match time.duration_since(std::time::UNIX_EPOCH) { + Ok(d) => i64::try_from(d.as_secs()).ok(), + Err(e) => i64::try_from(e.duration().as_secs()).ok().map(|s| -s), + } +} + +/// Builds a local [`FileEntry`] from its path and (possibly unreadable) metadata. +fn local_entry(path: &std::path::Path, meta: Option<&std::fs::Metadata>) -> FileEntry { + let name = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| path.to_string_lossy().into_owned()); + FileEntry { + name, + path: path.to_string_lossy().into_owned(), + size: meta.map(|m| m.len()).unwrap_or(0), + is_dir: meta.map(|m| m.is_dir()).unwrap_or(false), + modified: meta.and_then(|m| unix_secs(m.modified())), + created: meta.and_then(|m| unix_secs(m.created())), + } } // --------------------------------------------------------------------------- @@ -249,12 +293,7 @@ async fn do_list_dir( } else { &parent_str }; - entries.push(FileEntry { - name: "..".to_string(), - path: parent_str.to_string(), - size: 0, - is_dir: true, - }); + entries.push(FileEntry::parent(parent_str)); } for entry in read_dir { @@ -273,6 +312,8 @@ async fn do_list_dir( path: full_path, size: meta.size.unwrap_or(0), is_dir: ft.is_dir(), + modified: meta.mtime.map(i64::from), + created: None, }); } @@ -436,12 +477,7 @@ pub async fn list_local_dir(path: &str) -> anyhow::Result> { } else { &parent_str }; - entries.push(FileEntry { - name: "..".to_string(), - path: parent_str.to_string(), - size: 0, - is_dir: true, - }); + entries.push(FileEntry::parent(parent_str)); } while let Some(entry) = read_dir @@ -452,17 +488,11 @@ pub async fn list_local_dir(path: &str) -> anyhow::Result> { let file_type = entry.file_type().await.ok(); let is_dir = file_type.as_ref().map(|ft| ft.is_dir()).unwrap_or(false); let meta = entry.metadata().await.ok(); - let size = meta.as_ref().map(|m| m.len()).unwrap_or(0); - - let name = entry.file_name().to_string_lossy().into_owned(); - let path_str = entry.path().to_string_lossy().into_owned(); - entries.push(FileEntry { - name, - path: path_str, - size, - is_dir, - }); + let mut file = local_entry(&entry.path(), meta.as_ref()); + file.name = entry.file_name().to_string_lossy().into_owned(); + file.is_dir = is_dir; + entries.push(file); } // Sort: ".." first, then dirs, then files — case-insensitive alphabetically. @@ -483,6 +513,18 @@ pub async fn list_local_dir(path: &str) -> anyhow::Result> { Ok(entries) } +/// Stats each local path (following symlinks), e.g. files dropped onto the app from +/// the OS. Paths that cannot be read are left out. +pub async fn stat_local_paths(paths: &[String]) -> Vec { + let mut entries = Vec::with_capacity(paths.len()); + for path in paths { + if let Ok(meta) = tokio::fs::metadata(path).await { + entries.push(local_entry(std::path::Path::new(path), Some(&meta))); + } + } + entries +} + /// The roots the local file system can be browsed from: every drive letter on /// Windows, where `..` stops at the drive the pane is on, and `/` elsewhere. pub fn local_roots() -> Vec { @@ -538,6 +580,64 @@ mod tests { assert!(drive_roots(0).is_empty()); } + fn scratch_dir(tag: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("omnyssh-sftp-{tag}-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create scratch dir"); + dir + } + + #[tokio::test] + async fn local_listing_carries_modification_time() { + let dir = scratch_dir("list"); + std::fs::write(dir.join("a.txt"), b"hello").expect("write scratch file"); + + let entries = list_local_dir(&dir.to_string_lossy()) + .await + .expect("list scratch dir"); + let file = entries + .iter() + .find(|e| e.name == "a.txt") + .expect("file listed"); + assert_eq!(file.size, 5); + assert!(!file.is_dir); + assert!(file.modified.is_some_and(|t| t > 0)); + let parent = entries.first().expect("parent entry"); + assert_eq!(parent.name, ".."); + assert!(parent.modified.is_none() && parent.created.is_none()); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[tokio::test] + async fn stat_local_paths_tells_files_from_dirs_and_skips_missing() { + let dir = scratch_dir("stat"); + let file = dir.join("b.bin"); + std::fs::write(&file, b"xy").expect("write scratch file"); + let missing = dir.join("missing"); + + let paths = [ + file.to_string_lossy().into_owned(), + dir.to_string_lossy().into_owned(), + missing.to_string_lossy().into_owned(), + ]; + let entries = stat_local_paths(&paths).await; + assert_eq!(entries.len(), 2); + assert_eq!(entries[0].name, "b.bin"); + assert!(!entries[0].is_dir); + assert_eq!(entries[0].size, 2); + assert!(entries[1].is_dir); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn unix_secs_handles_times_before_the_epoch() { + let before = std::time::UNIX_EPOCH - Duration::from_secs(10); + assert_eq!(unix_secs(Ok(before)), Some(-10)); + assert_eq!(unix_secs(Ok(std::time::UNIX_EPOCH)), Some(0)); + assert_eq!(unix_secs(Err(std::io::Error::other("no"))), None); + } + #[cfg(windows)] #[test] fn the_system_drive_is_a_root() { diff --git a/crates/omnyssh-gui/src/bridge.rs b/crates/omnyssh-gui/src/bridge.rs index 0e518645..9fa158f8 100644 --- a/crates/omnyssh-gui/src/bridge.rs +++ b/crates/omnyssh-gui/src/bridge.rs @@ -458,6 +458,8 @@ mod tests { path: format!("/srv/{name}"), size: 0, is_dir: false, + modified: None, + created: None, } } diff --git a/crates/omnyssh-gui/src/commands/sftp.rs b/crates/omnyssh-gui/src/commands/sftp.rs index ab6d971d..988557be 100644 --- a/crates/omnyssh-gui/src/commands/sftp.rs +++ b/crates/omnyssh-gui/src/commands/sftp.rs @@ -12,7 +12,8 @@ use omnyssh_core::ssh::identity; use omnyssh_core::ssh::password::Prompter; use omnyssh_core::ssh::sftp::{ list_local_dir as core_list_local_dir, local_roots, - preview_local_file as core_preview_local_file, SftpCommand, SftpManager, + preview_local_file as core_preview_local_file, stat_local_paths as core_stat_local_paths, + SftpCommand, SftpManager, }; use crate::bridge; @@ -186,6 +187,18 @@ pub async fn list_local_dir(path: String) -> Result, CommandEr Ok(entries.iter().map(FileEntryDto::from).collect()) } +/// Stat local paths, e.g. files dropped onto a pane from the OS, so the frontend can +/// tell files from directories. Paths that cannot be read are left out. +#[tauri::command] +#[specta::specta] +pub async fn stat_local_paths(paths: Vec) -> Vec { + core_stat_local_paths(&paths) + .await + .iter() + .map(FileEntryDto::from) + .collect() +} + /// The roots the local pane can switch to: every drive letter on Windows, `/` /// elsewhere (tech-gui.md §4.2). #[tauri::command] diff --git a/crates/omnyssh-gui/src/dto.rs b/crates/omnyssh-gui/src/dto.rs index cf86c283..37a89d2f 100644 --- a/crates/omnyssh-gui/src/dto.rs +++ b/crates/omnyssh-gui/src/dto.rs @@ -243,6 +243,8 @@ pub struct SnippetDto { /// A file or directory in an SFTP panel listing (tech-gui.md §4.1). Maps from the /// core `FileEntry`; `path` is the absolute path the frontend marks entries by. +/// `modified`/`created` are Unix seconds, `null` when unknown; `created` is always +/// `null` for remote entries, since SFTP v3 does not report it. #[derive(Debug, Clone, Serialize, Deserialize, specta::Type)] #[serde(rename_all = "camelCase")] pub struct FileEntryDto { @@ -250,6 +252,8 @@ pub struct FileEntryDto { pub path: String, pub size: u64, pub is_dir: bool, + pub modified: Option, + pub created: Option, } /// Live progress for one SFTP upload/download (tech-gui.md §4.1). The GUI allocates @@ -561,6 +565,8 @@ impl From<&FileEntry> for FileEntryDto { path: entry.path.clone(), size: entry.size, is_dir: entry.is_dir, + modified: entry.modified, + created: entry.created, } } } @@ -945,38 +951,47 @@ mod tests { path: "/etc/omnyssh/config.toml".to_string(), size: 4096, is_dir: false, + modified: Some(1_700_000_000), + created: Some(1_600_000_000), }; let dto = FileEntryDto::from(&file); assert_eq!(dto.name, "config.toml"); assert_eq!(dto.path, "/etc/omnyssh/config.toml"); assert_eq!(dto.size, 4096); assert!(!dto.is_dir); + assert_eq!(dto.modified, Some(1_700_000_000)); + assert_eq!(dto.created, Some(1_600_000_000)); let dir = FileEntry { name: "..".to_string(), path: "/etc".to_string(), size: 0, is_dir: true, + modified: None, + created: None, }; let dto = FileEntryDto::from(&dir); assert!(dto.is_dir); assert_eq!(dto.size, 0); + assert!(dto.modified.is_none() && dto.created.is_none()); } #[test] fn file_entry_dto_uses_camel_case_is_dir_on_the_wire() { // The frontend reads `isDir` (tech-gui.md §4.1); a snake-case leak would - // silently render every entry as a file. + // silently render every entry as a file. Unknown times go out as `null`. let json = serde_json::to_string(&FileEntryDto::from(&FileEntry { name: "srv".to_string(), path: "/srv".to_string(), size: 0, is_dir: true, + modified: Some(1_700_000_000), + created: None, })) .expect("serialise FileEntryDto"); assert_eq!( json, - r#"{"name":"srv","path":"/srv","size":0,"isDir":true}"# + r#"{"name":"srv","path":"/srv","size":0,"isDir":true,"modified":1700000000,"created":null}"# ); } diff --git a/crates/omnyssh-gui/src/main.rs b/crates/omnyssh-gui/src/main.rs index 0148f6c9..228fc316 100644 --- a/crates/omnyssh-gui/src/main.rs +++ b/crates/omnyssh-gui/src/main.rs @@ -19,7 +19,7 @@ use commands::hosts::{delete_host, list_hosts, refresh_metrics, reload_hosts, sa use commands::keysetup::start_key_setup; use commands::sftp::{ list_local_dir, list_local_roots, preview_local_file, sftp_close, sftp_delete, sftp_download, - sftp_list, sftp_mkdir, sftp_open, sftp_preview, sftp_rename, sftp_upload, + sftp_list, sftp_mkdir, sftp_open, sftp_preview, sftp_rename, sftp_upload, stat_local_paths, }; use commands::snippets::{delete_snippet, execute_snippet, list_snippets, save_snippet}; use commands::terminal::{ @@ -115,6 +115,7 @@ fn specta_builder() -> Builder { sftp_close, list_local_dir, list_local_roots, + stat_local_paths, preview_local_file, start_key_setup, tunnel_start, diff --git a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts index 3d0457d2..0571154f 100644 --- a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts +++ b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts @@ -1,4 +1,4 @@ -import { expect, test, type Page } from '@playwright/test'; +import { expect, test, type Locator, type Page } from '@playwright/test'; // SFTP dual-pane vertical (tech-gui.md §3.2). e2e runs against the static SPA with // Tauri absent, so we stub `__TAURI_INTERNALS__` at the boundary (§6.4). The stub owns @@ -25,7 +25,16 @@ async function boot(page: Page, windows = false): Promise { // is observable mid-flight (the core is sequential — one transfer at a time). const completions: Array<() => void> = []; - type Entry = { name: string; path: string; size: number; isDir: boolean }; + type Entry = { + name: string; + path: string; + size: number; + isDir: boolean; + modified?: number | null; + created?: number | null; + }; + // 2024-06-15 12:00 UTC: mid-month and midday, so the year shows in any time zone. + const JUNE_2024 = 1718452800; const home = windows ? 'C:\\Users\\me' : '/home/user'; const roots = windows ? ['C:\\', 'D:\\', 'E:\\', 'Z:\\'] : ['/']; const local: Record = windows @@ -35,16 +44,27 @@ async function boot(page: Page, windows = false): Promise { } : { [home]: [ - { name: 'notes.txt', path: '/home/user/notes.txt', size: 24, isDir: false }, + { + name: 'notes.txt', + path: '/home/user/notes.txt', + size: 24, + isDir: false, + modified: JUNE_2024, + created: JUNE_2024 + }, { name: 'work', path: '/home/user/work', size: 0, isDir: true } ] }; - // Where each download was sent, for the test to read back. + // Where each transfer was sent, for the test to read back. const downloads: string[] = []; + const uploads: string[] = []; (win as { __downloads?: string[] }).__downloads = downloads; + (win as { __uploads?: string[] }).__uploads = uploads; + (win as { __fireEvent?: unknown }).__fireEvent = (event: string, payload: unknown) => + fireEvent(event, payload); const remote: Record = { '/': [ - { name: 'config.yml', path: '/config.yml', size: 64, isDir: false }, + { name: 'config.yml', path: '/config.yml', size: 64, isDir: false, modified: JUNE_2024 }, { name: 'var', path: '/var', size: 0, isDir: true } ] }; @@ -78,6 +98,11 @@ async function boot(page: Page, windows = false): Promise { (win as { __completeTransfer?: () => void }).__completeTransfer = () => completions.shift()?.(); (win as { __TAURI_INTERNALS__: unknown }).__TAURI_INTERNALS__ = { + // `getCurrentWebview()` reads these to scope OS drag-drop listeners. + metadata: { + currentWindow: { label: 'main' }, + currentWebview: { label: 'main', windowLabel: 'main' } + }, invoke: (cmd: string, args: Record) => { if (cmd.startsWith('plugin:path')) return Promise.resolve(home); switch (cmd) { @@ -112,6 +137,7 @@ async function boot(page: Page, windows = false): Promise { } case 'sftp_upload': { const { sessionId, remote: dest } = args as { sessionId: number; remote: string }; + uploads.push(dest); const tid = ++nextTransfer; setTimeout(() => fireEvent('transfer-progress', { sessionId, transferId: tid, done: 4, total: 8 }), 0); completions.push(() => { @@ -133,6 +159,15 @@ async function boot(page: Page, windows = false): Promise { } case 'sftp_close': return Promise.resolve(null); + // Files dropped from the OS: a path ending in `/` stands for a folder. + case 'stat_local_paths': + return Promise.resolve( + (args.paths as string[]).map((p) => { + const isDir = p.endsWith('/'); + const path = isDir ? p.slice(0, -1) : p; + return { name: baseName(path), path, size: isDir ? 0 : 3, isDir, modified: null, created: null }; + }) + ); case 'plugin:event|listen': { const { event, handler } = args as { event: string; handler: number }; (listeners[event] ||= []).push(handler); @@ -203,6 +238,102 @@ test('round-trip: upload a local file to the remote, then download a remote file await expect(localPane.getByText('config.yml')).toBeVisible(); }); +test('the panes show modification times, and creation times on the local side only', async ({ + page +}) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('notes.txt')).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + await expect(localPane.getByTitle(/^Modified .*2024/)).toBeVisible(); + await expect(localPane.getByTitle(/^Created .*2024/)).toBeVisible(); + await expect(remotePane.getByTitle(/^Modified .*2024/)).toBeVisible(); + await expect(remotePane.getByText('Created', { exact: true })).toHaveCount(0); +}); + +async function dragOnto(page: Page, from: Locator, to: Locator): Promise { + const a = await from.boundingBox(); + const b = await to.boundingBox(); + if (!a || !b) throw new Error('drag endpoints not laid out'); + await page.mouse.move(a.x + a.width / 2, a.y + a.height / 2); + await page.mouse.down(); + // Several steps: the drag only starts past a small threshold, then tracks the target. + await page.mouse.move(a.x + a.width / 2 + 20, a.y + a.height / 2, { steps: 4 }); + await page.mouse.move(b.x + b.width / 2, b.y + b.height / 2, { steps: 8 }); + await page.mouse.up(); +} + +test('drag and drop: a local file dropped on the remote pane uploads there', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + await dragOnto(page, localPane.getByTitle('notes.txt', { exact: true }), remotePane.getByText('config.yml')); + await expect(page.getByLabel('transfer progress')).toBeVisible(); + const uploads = await page.evaluate(() => (window as unknown as { __uploads: string[] }).__uploads); + expect(uploads).toEqual(['/notes.txt']); + // The drag ended away from the row it started on, so nothing was previewed. + await expect(page.getByRole('dialog', { name: 'File preview' })).toHaveCount(0); + + await page.evaluate(() => (window as unknown as { __completeTransfer: () => void }).__completeTransfer()); + await expect(remotePane.getByText('notes.txt')).toBeVisible(); +}); + +test('drag and drop: a remote file dropped on a local folder downloads into it', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('work')).toBeVisible(); + + await dragOnto(page, remotePane.getByTitle('config.yml', { exact: true }), localPane.getByTitle('work', { exact: true })); + await expect(page.getByLabel('transfer progress')).toBeVisible(); + const downloads = await page.evaluate(() => (window as unknown as { __downloads: string[] }).__downloads); + expect(downloads).toEqual(['/home/user/work/config.yml']); +}); + +test('drag and drop: dropping back on the same pane transfers nothing', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + await expect(localPane.getByText('work')).toBeVisible(); + + await dragOnto(page, localPane.getByTitle('notes.txt', { exact: true }), localPane.getByTitle('work', { exact: true })); + await expect(page.getByLabel('transfer progress')).toHaveCount(0); + const uploads = await page.evaluate(() => (window as unknown as { __uploads: string[] }).__uploads); + expect(uploads).toEqual([]); +}); + +test('drag and drop: files dropped from the OS onto the remote pane upload, folders are refused', async ({ + page +}) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + const box = await remotePane.getByText('config.yml').boundingBox(); + if (!box) throw new Error('remote pane not laid out'); + const position = { x: box.x + box.width / 2, y: box.y + box.height / 2 }; + await page.evaluate( + ({ position }) => { + const fire = (window as unknown as { __fireEvent: (e: string, p: unknown) => void }).__fireEvent; + fire('tauri://drag-drop', { paths: ['/tmp/photo.png', '/tmp/album/'], position }); + }, + { position } + ); + + await expect(page.getByLabel('transfer progress')).toBeVisible(); + const uploads = await page.evaluate(() => (window as unknown as { __uploads: string[] }).__uploads); + expect(uploads).toEqual(['/photo.png']); + await expect(page.getByText('Folders cannot be uploaded yet')).toBeVisible(); +}); + test('an inactive tab’s modal never overlays another entity (§2 exactly-one-active)', async ({ page }) => { diff --git a/crates/omnyssh-gui/ui/src/lib/bindings.ts b/crates/omnyssh-gui/ui/src/lib/bindings.ts index e497bf6c..1444cb2f 100644 --- a/crates/omnyssh-gui/ui/src/lib/bindings.ts +++ b/crates/omnyssh-gui/ui/src/lib/bindings.ts @@ -294,6 +294,13 @@ async listLocalDir(path: string) : Promise> async listLocalRoots() : Promise { return await TAURI_INVOKE("list_local_roots"); }, +/** + * Stat local paths, e.g. files dropped onto a pane from the OS, so the frontend can + * tell files from directories. Paths that cannot be read are left out. + */ +async statLocalPaths(paths: string[]) : Promise { + return await TAURI_INVOKE("stat_local_paths", { paths }); +}, /** * Read up to 4 KiB of a local file as UTF-8 for preview (tech-gui.md §4.2). */ @@ -515,8 +522,10 @@ export type Error = { message: string } /** * A file or directory in an SFTP panel listing (tech-gui.md §4.1). Maps from the * core `FileEntry`; `path` is the absolute path the frontend marks entries by. + * `modified`/`created` are Unix seconds, `null` when unknown; `created` is always + * `null` for remote entries, since SFTP v3 does not report it. */ -export type FileEntryDto = { name: string; path: string; size: number; isDir: boolean } +export type FileEntryDto = { name: string; path: string; size: number; isDir: boolean; modified: number | null; created: number | null } /** * Preview bytes for a remote file (tech-gui.md §4.3). Stamped with `sessionId`; the * core `FilePreviewReady` carries only the path + content (§3.4). diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts index cf40818d..a3086979 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts @@ -165,6 +165,11 @@ export async function listLocalDir(path: string): Promise { return res.data; } +/** Stat local paths (e.g. files dropped from the OS); unreadable ones are left out. */ +export async function statLocalPaths(paths: string[]): Promise { + return commands.statLocalPaths(paths); +} + /** The roots the local pane can switch to: drive letters on Windows, `/` elsewhere. */ export async function listLocalRoots(): Promise { return commands.listLocalRoots(); diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/subscribe.test.ts b/crates/omnyssh-gui/ui/src/lib/ipc/subscribe.test.ts index 8775343d..1b28457a 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/subscribe.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/subscribe.test.ts @@ -139,7 +139,16 @@ describe('startEventBridge', () => { payload: { sessionId: 11, path: '/srv', - entries: [{ name: 'app.log', path: '/srv/app.log', size: 12, isDir: false }] + entries: [ + { + name: 'app.log', + path: '/srv/app.log', + size: 12, + isDir: false, + modified: null, + created: null + } + ] } }); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte index 0a74a3cd..b8da0c9c 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte @@ -2,34 +2,58 @@ // One side of the dual-pane SFTP browser (tech-gui.md §3.2): a current-path header // with a parent-supplied toolbar, then the entry list. Clicking a directory (or the // `..` row) navigates; clicking a file previews; the leading checkbox marks it for a - // batch transfer/delete. Semantic tokens only — no colour literals (§5.1). + // batch transfer/delete. A press on a row may start a drag (the parent owns the drag + // and marks the pane `dropActive` while it is the target); `data-pane` and + // `data-dir-path` are how the parent finds the pane and folder under the pointer. + // Semantic tokens only — no colour literals (§5.1). import type { Snippet } from 'svelte'; import { Icon } from '$lib/theme'; import type { FileEntryDto } from '$lib/bindings'; - import { formatBytes, type Pane } from '$lib/stores/sftp'; + import { formatBytes, formatDate, type Pane, type PaneSide } from '$lib/stores/sftp'; let { title, + side, pane, + showCreated = false, + dropActive = false, + dropDir, onNavigate, onToggleMark, onPreview, + onDragStart, toolbar }: { title: string; + side: PaneSide; pane: Pane; + /** Show the creation-time column (local only: SFTP v3 does not report it). */ + showCreated?: boolean; + /** The pane is the target of a drag in progress. */ + dropActive?: boolean; + /** The folder row under the pointer while this pane is the drop target. */ + dropDir?: string; onNavigate: (entry: FileEntryDto) => void; onToggleMark: (path: string) => void; onPreview: (entry: FileEntryDto) => void; + onDragStart?: (entry: FileEntryDto, event: PointerEvent) => void; toolbar?: Snippet; } = $props(); const rowBase = 'flex w-full min-w-0 items-center gap-2 rounded px-2 py-1.5 text-left text-sm transition ' + 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-focus'; + // Date columns appear once the pane is wide enough to keep names readable. + const modifiedCol = 'hidden w-36 shrink-0 text-right lg:inline'; + const createdCol = 'hidden w-36 shrink-0 text-right xl:inline'; -
+

Empty directory

{:else} +
    {#each pane.entries as entry, i (i)} {@const isParent = entry.name === '..'} {@const marked = pane.marked.has(entry.path)} -
  • + {@const dropHere = dropActive && entry.isDir && dropDir === entry.path} +
  • {#if isParent} {:else} @@ -81,6 +119,7 @@ class="{rowBase} text-muted hover:bg-surface-inset hover:text-fg" title={entry.name} onclick={() => (entry.isDir ? onNavigate(entry) : onPreview(entry))} + onpointerdown={(e) => onDragStart?.(entry, e)} > @@ -89,6 +128,22 @@ {#if !entry.isDir} {formatBytes(entry.size)} {/if} + {#if !isParent} + {#if showCreated} + + {formatDate(entry.created)} + + {/if} + + {formatDate(entry.modified)} + + {/if}
    • {/each} diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index 6954dbed..99d40e2b 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -7,13 +7,21 @@ // remote uses sftp_list (arrives as an event). Semantic tokens only (§5.1). import { onMount, onDestroy } from 'svelte'; import { homeDir } from '@tauri-apps/api/path'; + import { getCurrentWebview } from '@tauri-apps/api/webview'; import { Icon } from '$lib/theme'; import Modal from '$lib/components/Modal.svelte'; import Select from '$lib/components/Select.svelte'; import SftpPane from './SftpPane.svelte'; import type { FileEntryDto } from '$lib/bindings'; import { sessions, type Session } from '$lib/stores/sessions'; - import { sftp, markedEntries, formatBytes, rootOf, type PaneSide } from '$lib/stores/sftp'; + import { + sftp, + markedEntries, + dragPayload, + formatBytes, + rootOf, + type PaneSide + } from '$lib/stores/sftp'; import { lastError } from '$lib/stores/notifications'; import { sftpOpen, @@ -27,7 +35,8 @@ sftpPreview, listLocalDir, listLocalRoots, - previewLocalFile + previewLocalFile, + statLocalPaths } from '$lib/ipc/commands'; let { session, active }: { session: Session; active: boolean } = $props(); @@ -226,30 +235,174 @@ outbox = [...outbox, ...actions]; } - function upload(): void { + // `dir` defaults to the other pane's current directory; a drop onto a folder row + // passes that folder instead. + function upload(files: FileEntryDto[], dir = view?.remote.path): void { const id = backendId; - if (id == null || !view) return; - const dir = view.remote.path; + if (id == null || !view || dir == null) return; enqueue( - ...localMarkedFiles.map((file) => () => { + ...files.map((file) => () => { sftp.pushOp(id, { kind: 'upload', name: file.name, refresh: 'remote' }); void sftpUpload(id, file.path, joinRemote(dir, file.name)).catch(onDispatchError(id)); }) ); } - function download(): void { + function download(files: FileEntryDto[], dir = view?.local.path): void { const id = backendId; - if (id == null || !view) return; - const dir = view.local.path; + if (id == null || !view || dir == null) return; enqueue( - ...remoteMarkedFiles.map((file) => () => { + ...files.map((file) => () => { sftp.pushOp(id, { kind: 'download', name: file.name, refresh: 'local' }); void sftpDownload(id, joinLocal(dir, file.name), file.path).catch(onDispatchError(id)); }) ); } + // --- Drag and drop ----------------------------------------------------------------- + // Pane to pane uses pointer events, not HTML5 drag and drop: Tauri's native file-drop + // handler (on by default, and needed for drops from the OS below) swallows HTML5 drag + // events in WebView2. A press becomes a drag only past DRAG_THRESHOLD px, so a click + // still navigates or previews. + const DRAG_THRESHOLD = 5; + + interface DropTarget { + side: PaneSide; + /** The directory the drop lands in: a folder row under the pointer, else the + * pane's current directory. */ + dir: string; + /** Set when `dir` is a folder row, for its highlight. */ + row?: string; + } + + let press: { side: PaneSide; entry: FileEntryDto; x: number; y: number } | null = null; + let drag = $state<{ + from: PaneSide; + files: FileEntryDto[]; + x: number; + y: number; + target: DropTarget | null; + } | null>(null); + // A drag that ends on the row it started on would otherwise click it. + let swallowClick = false; + // A drag from the OS hovering this view, for the remote pane's highlight. + let osDrop = $state(null); + + /** The pane (and folder row, if any) under a viewport point. */ + function dropTargetAt(x: number, y: number): DropTarget | null { + if (!view) return null; + const el = document.elementFromPoint(x, y); + const paneEl = el?.closest('[data-pane]'); + const side = paneEl?.dataset.pane; + if (side !== 'local' && side !== 'remote') return null; + const row = el?.closest('[data-dir-path]')?.dataset.dirPath; + return { side, dir: row ?? view[side].path, row }; + } + + function startPress(side: PaneSide, entry: FileEntryDto, e: PointerEvent): void { + if (e.button !== 0 || !view) return; + press = { side, entry, x: e.clientX, y: e.clientY }; + } + + function onPointerMove(e: PointerEvent): void { + if (drag) { + const target = dropTargetAt(e.clientX, e.clientY); + drag = { + ...drag, + x: e.clientX, + y: e.clientY, + target: target && target.side !== drag.from ? target : null + }; + return; + } + if (!press || !view) return; + if (Math.hypot(e.clientX - press.x, e.clientY - press.y) < DRAG_THRESHOLD) return; + const files = dragPayload(view[press.side], press.entry); + const from = press.side; + press = null; + if (files.length === 0) return; + drag = { from, files, x: e.clientX, y: e.clientY, target: null }; + } + + function onPointerUp(): void { + press = null; + const done = drag; + if (!done) return; + drag = null; + swallowClick = true; + setTimeout(() => (swallowClick = false), 0); + if (!done.target) return; + if (done.from === 'local') upload(done.files, done.target.dir); + else download(done.files, done.target.dir); + } + + function onKeyDown(e: KeyboardEvent): void { + if (e.key === 'Escape' && drag) { + drag = null; + press = null; + } + } + + // Drops from the OS (Explorer, Finder): Tauri reports physical pixels and absolute + // paths. Only the remote pane accepts them; every SFTP view stays mounted, so only + // the visible one reacts. + onMount(() => { + let unlisten: (() => void) | undefined; + let webview: ReturnType; + try { + webview = getCurrentWebview(); + } catch { + // Off the Tauri runtime there is no webview to drop onto; pane to pane still works. + return; + } + void webview + .onDragDropEvent((event) => { + if (!active || !view) return; + const p = event.payload; + if (p.type === 'leave') { + osDrop = null; + return; + } + const ratio = window.devicePixelRatio || 1; + const target = dropTargetAt(p.position.x / ratio, p.position.y / ratio); + const remote = target?.side === 'remote' ? target : null; + if (p.type === 'drop') { + osDrop = null; + if (remote) void uploadDropped(p.paths, remote.dir); + } else { + osDrop = remote; + } + }) + .then((fn) => { + if (destroyed) fn(); + else unlisten = fn; + }) + .catch(() => {}); + return () => unlisten?.(); + }); + + async function uploadDropped(paths: string[], dir: string): Promise { + const entries = await statLocalPaths(paths).catch((err) => { + lastError.set(errMsg(err)); + return [] as FileEntryDto[]; + }); + const files = entries.filter((e) => !e.isDir); + if (files.length < paths.length) { + lastError.set( + entries.some((e) => e.isDir) + ? 'Folders cannot be uploaded yet; only the dropped files were sent.' + : 'Some dropped items could not be read.' + ); + } + upload(files, dir); + } + + function guarded(fn: (arg: T) => void): (arg: T) => void { + return (arg) => { + if (!swallowClick) fn(arg); + }; + } + function remove(): void { const id = backendId; if (id == null) return; @@ -313,9 +466,36 @@ 'focus-visible:ring-2 focus-visible:ring-focus placeholder:text-faint'; + { + press = null; + drag = null; + }} + onkeydown={onKeyDown} +/> + +{#if drag} + + +{/if} + -
    + panes below them. The select-none while dragging keeps the drag from selecting text. --> +
    {#if openError}

    Could not open SFTP on {session.hostName}

    @@ -329,10 +509,15 @@
    navigate('local', e)} + showCreated + dropActive={drag?.target?.side === 'local'} + dropDir={drag?.target?.side === 'local' ? drag.target.row : undefined} + onNavigate={guarded((e) => navigate('local', e))} onToggleMark={(p) => toggleMark('local', p)} - onPreview={(e) => preview('local', e)} + onPreview={guarded((e) => preview('local', e))} + onDragStart={(entry, e) => startPress('local', entry, e)} > {#snippet toolbar()} {#if roots.length > 1} @@ -356,7 +541,7 @@ class={toolBtn} title="Upload marked files to the remote directory" disabled={localMarkedFiles.length === 0} - onclick={upload} + onclick={() => upload(localMarkedFiles)} > Upload @@ -379,10 +564,14 @@ navigate('remote', e)} + dropActive={drag?.target?.side === 'remote' || osDrop != null} + dropDir={drag?.target?.side === 'remote' ? drag.target.row : osDrop?.row} + onNavigate={guarded((e) => navigate('remote', e))} onToggleMark={(p) => toggleMark('remote', p)} - onPreview={(e) => preview('remote', e)} + onPreview={guarded((e) => preview('remote', e))} + onDragStart={(entry, e) => startPress('remote', entry, e)} > {#snippet toolbar()}
    {/if} @@ -539,9 +535,9 @@
      @@ -129,14 +124,6 @@ {formatBytes(entry.size)} {/if} {#if !isParent} - {#if showCreated} - - {formatDate(entry.created)} - - {/if} >, dir = view?.remote.path): void { const id = backendId; if (id == null || !view || dir == null) return; enqueue( @@ -370,7 +370,7 @@ const remote = target?.side === 'remote' ? target : null; if (p.type === 'drop') { osDrop = null; - if (remote) void uploadDropped(p.paths, remote.dir); + if (remote) uploadDropped(p.paths, remote.dir); } else { osDrop = remote; } @@ -383,14 +383,12 @@ return () => unlisten?.(); }); - async function uploadDropped(paths: string[], dir: string): Promise { - const entries = await statLocalPaths(paths).catch((err) => { - lastError.set(errMsg(err)); - return [] as FileEntryDto[]; - }); - // Folders upload whole, contents and all (the core walks them). - if (entries.length < paths.length) lastError.set('Some dropped items could not be read.'); - upload(entries, dir); + // Dropped paths upload as they are: the core tells files from folders and walks the + // folders. + function uploadDropped(paths: string[], dir: string): void { + const items = paths.map((path) => ({ path, name: baseName(path) })); + if (items.some((item) => !item.name)) lastError.set('A drive or volume cannot be uploaded.'); + upload(items.filter((item) => item.name), dir); } function guarded(fn: (arg: T) => void): (arg: T) => void { @@ -507,7 +505,6 @@ title="Local" side="local" pane={view.local} - showCreated dropActive={drag?.target?.side === 'local'} dropDir={drag?.target?.side === 'local' ? drag.target.row : undefined} onNavigate={guarded((e) => navigate('local', e))} diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts index b244b887..56821991 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts @@ -14,6 +14,7 @@ import { formatDate, dragPayload, rootOf, + baseName, type Pane, type SftpSession } from './sftp'; @@ -22,7 +23,7 @@ import { // it is unit-testable without a Tauri runtime (tech-gui.md §3.2, §6.4). function entry(name: string, isDir = false, size = 0): FileEntryDto { - return { name, path: `/srv/${name}`, size, isDir, modified: null, created: null }; + return { name, path: `/srv/${name}`, size, isDir, modified: null }; } function paneWith(entries: FileEntryDto[], marked: string[] = []): Pane { @@ -214,6 +215,20 @@ describe('rootOf', () => { }); }); +describe('baseName', () => { + it('takes the last component of a dropped path', () => { + expect(baseName('/tmp/album/')).toBe('album'); + expect(baseName('/home/me/back\\slash.txt')).toBe('back\\slash.txt'); + expect(baseName('C:\\Users\\me\\photo.png')).toBe('photo.png'); + }); + + it('has no name for a root', () => { + expect(baseName('/')).toBe(''); + expect(baseName('C:\\')).toBe(''); + expect(baseName('d:')).toBe(''); + }); +}); + describe('sftp store', () => { it('keeps concurrent sessions isolated and prunes on close', () => { sftp.open(1, 'web-1'); diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts index b741c048..61f6ff58 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts @@ -113,6 +113,15 @@ export function rootOf(path: string, roots: string[]): string { return roots.find((root) => lower.startsWith(root.toLowerCase())) ?? ''; } +/** The name a local path is uploaded under: its last component, or '' for a root + * ('/', 'C:\\'), which has no name to give it on the server. A path with no '/' is a + * Windows one, where '\\' separates; elsewhere '\\' is part of a name. */ +export function baseName(path: string): string { + const sep = path.includes('/') ? '/' : '\\'; + const name = path.split(sep).filter(Boolean).at(-1) ?? ''; + return sep === '\\' && /^[a-z]:$/i.test(name) ? '' : name; +} + /** Human-readable byte size for a listing row or a transfer bar. */ export function formatBytes(bytes: number): string { if (bytes < 1024) return `${bytes} B`; diff --git a/crates/omnyssh/src/app/file_manager.rs b/crates/omnyssh/src/app/file_manager.rs index f71498d5..930514b2 100644 --- a/crates/omnyssh/src/app/file_manager.rs +++ b/crates/omnyssh/src/app/file_manager.rs @@ -695,7 +695,6 @@ mod tests { size: 0, is_dir: false, modified: None, - created: None, } } From d576e8d73f354f2b0365bbb4094e6ece427b3a6f Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 22:25:05 +0400 Subject: [PATCH 06/44] fix(core): keep folder downloads inside the destination --- crates/omnyssh-core/src/ssh/sftp.rs | 157 +++++++++++++++++++++++++--- 1 file changed, 140 insertions(+), 17 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index 47910500..ec74aad7 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -319,10 +319,55 @@ fn join_remote(dir: &str, name: &str) -> String { } } -/// A directory entry name that is safe to join onto a destination: not `.` or `..` -/// and free of separators, so a hostile server cannot steer a download elsewhere. -fn is_plain_name(name: &str) -> bool { - !name.is_empty() && name != "." && name != ".." && !name.contains(['/', '\\', '\0']) +/// Whether a name the server gave can be created locally as itself: one plain path +/// component, so it neither climbs out of the destination nor, as Windows' `C:x` does, +/// replaces it. +fn is_safe_name(name: &str) -> bool { + let mut parts = std::path::Path::new(name).components(); + let single = matches!( + (parts.next(), parts.next()), + (Some(std::path::Component::Normal(part)), None) if part == name + ); + single && !name.contains('\0') && (!cfg!(windows) || is_windows_name(name)) +} + +/// Windows' own rules: no `:` (drives, alternate streams) or other reserved character, +/// no trailing dot or space (Win32 drops them, so `a.` would overwrite `a`), and no +/// device name, with or without an extension (`aux.c` opens the AUX device). +fn is_windows_name(name: &str) -> bool { + if name.chars().any(|c| c < ' ' || "<>:\"/\\|?*".contains(c)) || name.ends_with(['.', ' ']) { + return false; + } + let stem = name.split('.').next().unwrap_or(name).trim_end_matches(' '); + let upper = stem.to_ascii_uppercase(); + let device = match upper.get(..3) { + Some("CON" | "PRN" | "AUX" | "NUL") => upper.len() == 3, + Some("COM" | "LPT") => { + let port = &upper[3..]; + port.chars().count() == 1 && "0123456789\u{b9}\u{b2}\u{b3}".contains(port) + } + _ => false, + }; + !device +} + +/// `name` from the server joined onto the local folder `dir`, unless it would be +/// anything but a direct child of `dir`. +fn join_local(dir: &std::path::Path, name: &str) -> Option { + let path = dir.join(name); + (is_safe_name(name) && path.parent() == Some(dir)).then_some(path) +} + +/// Rejects a download destination whose own name could not be created as itself: the +/// frontends build it from the name the server listed. +fn check_local_name(local: &str) -> anyhow::Result<()> { + let name = std::path::Path::new(local) + .file_name() + .and_then(|n| n.to_str()); + if !name.is_some_and(is_safe_name) { + anyhow::bail!("'{local}' is not a name that can be created here"); + } + Ok(()) } /// Rejects a transfer whose local path climbs out with `..` or either path holds a @@ -409,9 +454,6 @@ async fn plan_local_tree(root: &std::path::Path, dst_root: &str) -> anyhow::Resu .with_context(|| format!("read local dir '{}'", src.display()))? { let name = entry.file_name().to_string_lossy().into_owned(); - if !is_plain_name(&name) { - continue; - } // `DirEntry::file_type` does not follow symlinks. let file_type = entry .file_type() @@ -433,7 +475,7 @@ async fn plan_local_tree(root: &std::path::Path, dst_root: &str) -> anyhow::Resu } /// Walks the remote folder `root`, mapping it onto the local folder `dst_root`. -/// Symlinks and special files are skipped, as are names that are not plain. +/// Symlinks and special files are skipped, as are names that cannot be created here. async fn plan_remote_tree( sftp: &russh_sftp::client::SftpSession, root: &str, @@ -449,16 +491,16 @@ async fn plan_remote_tree( plan.dirs.push(dst.to_string_lossy().into_owned()); for entry in read_dir { let name = entry.file_name(); - if !is_plain_name(&name) { + let Some(local) = join_local(&dst, &name) else { continue; - } + }; let file_type = entry.file_type(); if file_type.is_dir() { - stack.push((join_remote(&src, &name), dst.join(&name))); + stack.push((join_remote(&src, &name), local)); } else if file_type.is_file() { plan.files.push(PlannedFile { src: join_remote(&src, &name), - dst: dst.join(&name).to_string_lossy().into_owned(), + dst: local.to_string_lossy().into_owned(), size: entry.metadata().size.unwrap_or(0), }); } @@ -476,6 +518,7 @@ async fn do_download( event_tx: &mpsc::Sender, ) -> anyhow::Result<()> { check_paths(local, remote)?; + check_local_name(local)?; // Size for progress and file-or-folder (best-effort: a failed stat downloads as a // file, and the open below reports the real error). @@ -797,16 +840,96 @@ mod tests { } #[test] - fn plain_names_exclude_dots_and_separators() { - assert!(is_plain_name("report.txt")); - assert!(is_plain_name("..hidden")); - for name in ["", ".", "..", "a/b", "a\\b", "a\0b"] { - assert!(!is_plain_name(name), "{name:?} should not be plain"); + fn a_server_name_must_be_one_plain_component() { + for name in ["report.txt", "..hidden", "with space", "caf\u{e9}.txt"] { + assert!(is_safe_name(name), "{name:?} should be safe"); + } + for name in ["", ".", "..", "a/b", "/abs", "dir/", "a\0b"] { + assert!(!is_safe_name(name), "{name:?} should not be safe"); } assert_eq!(join_remote("/", "x"), "/x"); assert_eq!(join_remote("/srv", "x"), "/srv/x"); } + #[test] + fn windows_names_refuse_drives_devices_and_trimmed_endings() { + for name in [ + "C:x", + "D:", + "a:b", + "..\\x", + "a\\b", + "CON", + "aux.c", + "NUL.tar.gz", + "Com1", + "lpt9.txt", + "COM\u{b9}", + "CON .txt", + "a.", + "a ", + "a Date: Thu, 1 Oct 2026 23:41:22 +0400 Subject: [PATCH 07/44] fix(core): skip and report what a folder transfer leaves out --- crates/omnyssh-core/src/ssh/sftp.rs | 608 ++++++++++++++++++++++------ 1 file changed, 487 insertions(+), 121 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index ec74aad7..42883ffc 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -6,9 +6,12 @@ //! All operations are non-blocking from the UI perspective. //! Progress is reported via [`CoreEvent::FileTransferProgress`]. +use std::future::Future; +use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::Context; +use russh_sftp::protocol::{FileAttributes, FileType}; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::sync::mpsc; use tokio::time; @@ -70,13 +73,17 @@ fn unix_secs(time: std::io::Result) -> Option { pub enum SftpCommand { /// List the entries in a remote directory. ListDir(String), - /// Download a remote file to a local path. + /// Download a remote file, or a remote folder with everything in it, to a local + /// path. An existing folder is merged into and an existing file replaced. Inside a + /// folder, symlinks, special files and names this system cannot create are + /// skipped; the transfer goes on past them and past failed files, and its + /// [`CoreEvent::SftpOpDone`] error counts them. Download { remote: String, local: String, transfer_id: TransferId, }, - /// Upload a local file to a remote path. + /// Upload a local file or folder to a remote path, by the rules of `Download`. Upload { local: String, remote: String, @@ -190,7 +197,7 @@ async fn sftp_task_loop( Err(e) => { let _ = event_tx .send(CoreEvent::SftpDisconnected { - reason: format!("ListDir failed: {e}"), + reason: format!("ListDir failed: {e:#}"), }) .await; } @@ -203,7 +210,7 @@ async fn sftp_task_loop( } => { let result = do_download(&sftp, &remote, &local, transfer_id, &event_tx) .await - .map_err(|e| e.to_string()); + .map_err(|e| format!("{e:#}")); let _ = event_tx.send(CoreEvent::SftpOpDone { result }).await; } @@ -214,7 +221,7 @@ async fn sftp_task_loop( } => { let result = do_upload(&local, &sftp, &remote, transfer_id, &event_tx) .await - .map_err(|e| e.to_string()); + .map_err(|e| format!("{e:#}")); let _ = event_tx.send(CoreEvent::SftpOpDone { result }).await; } @@ -323,7 +330,7 @@ fn join_remote(dir: &str, name: &str) -> String { /// component, so it neither climbs out of the destination nor, as Windows' `C:x` does, /// replaces it. fn is_safe_name(name: &str) -> bool { - let mut parts = std::path::Path::new(name).components(); + let mut parts = Path::new(name).components(); let single = matches!( (parts.next(), parts.next()), (Some(std::path::Component::Normal(part)), None) if part == name @@ -353,7 +360,7 @@ fn is_windows_name(name: &str) -> bool { /// `name` from the server joined onto the local folder `dir`, unless it would be /// anything but a direct child of `dir`. -fn join_local(dir: &std::path::Path, name: &str) -> Option { +fn join_local(dir: &Path, name: &str) -> Option { let path = dir.join(name); (is_safe_name(name) && path.parent() == Some(dir)).then_some(path) } @@ -361,9 +368,7 @@ fn join_local(dir: &std::path::Path, name: &str) -> Option { /// Rejects a download destination whose own name could not be created as itself: the /// frontends build it from the name the server listed. fn check_local_name(local: &str) -> anyhow::Result<()> { - let name = std::path::Path::new(local) - .file_name() - .and_then(|n| n.to_str()); + let name = Path::new(local).file_name().and_then(|n| n.to_str()); if !name.is_some_and(is_safe_name) { anyhow::bail!("'{local}' is not a name that can be created here"); } @@ -373,7 +378,7 @@ fn check_local_name(local: &str) -> anyhow::Result<()> { /// Rejects a transfer whose local path climbs out with `..` or either path holds a /// null byte. fn check_paths(local: &str, remote: &str) -> anyhow::Result<()> { - if std::path::Path::new(local) + if Path::new(local) .components() .any(|c| c == std::path::Component::ParentDir) { @@ -385,9 +390,41 @@ fn check_paths(local: &str, remote: &str) -> anyhow::Result<()> { Ok(()) } -/// One file of a folder transfer. +// --------------------------------------------------------------------------- +// Transfers +// --------------------------------------------------------------------------- + +/// How deep a folder transfer goes: past any real tree, short of one a server makes +/// up to never end. +const MAX_DEPTH: usize = 64; + +/// What a folder transfer left out. It goes on past every skip and failure and +/// reports them in its one result. +#[derive(Debug, Default)] +struct Skipped { + count: usize, + first: Option, +} + +impl Skipped { + fn add(&mut self, path: &str, reason: impl std::fmt::Display) { + self.count += 1; + self.first + .get_or_insert_with(|| format!("'{path}': {reason}")); + } + + fn into_result(self) -> anyhow::Result<()> { + let Some(first) = self.first else { + return Ok(()); + }; + let items = if self.count == 1 { "item" } else { "items" }; + anyhow::bail!("{} {items} skipped or failed, first {first}", self.count) + } +} + +/// One folder or file of a folder transfer. #[derive(Debug, PartialEq)] -struct PlannedFile { +struct Planned { src: String, dst: String, size: u64, @@ -397,8 +434,8 @@ struct PlannedFile { /// the progress bar covers the whole folder. `dirs` lists parents before children. #[derive(Debug, Default)] struct TreePlan { - dirs: Vec, - files: Vec, + dirs: Vec, + files: Vec, } impl TreePlan { @@ -439,70 +476,122 @@ impl<'a> Progress<'a> { } /// Walks the local folder `root`, mapping it onto the remote folder `dst_root`. -/// Symlinks and special files are skipped, so a link cycle cannot loop forever. -async fn plan_local_tree(root: &std::path::Path, dst_root: &str) -> anyhow::Result { +/// Nothing is followed: a symlink is skipped, as is anything else that is not a +/// plain file or folder. +async fn plan_local_tree( + root: &str, + dst_root: &str, + skipped: &mut Skipped, +) -> anyhow::Result { let mut plan = TreePlan::default(); - let mut stack = vec![(root.to_path_buf(), dst_root.to_string())]; - while let Some((src, dst)) = stack.pop() { - let mut read_dir = tokio::fs::read_dir(&src) - .await - .with_context(|| format!("read local dir '{}'", src.display()))?; - plan.dirs.push(dst.clone()); - while let Some(entry) = read_dir - .next_entry() - .await - .with_context(|| format!("read local dir '{}'", src.display()))? - { - let name = entry.file_name().to_string_lossy().into_owned(); - // `DirEntry::file_type` does not follow symlinks. - let file_type = entry - .file_type() - .await - .with_context(|| format!("stat '{}'", entry.path().display()))?; - if file_type.is_dir() { - stack.push((entry.path(), join_remote(&dst, &name))); + let mut stack = vec![(PathBuf::from(root), dst_root.to_string(), 0)]; + while let Some((src, dst, depth)) = stack.pop() { + // Every name on the way is UTF-8, so the path converts losslessly. + let src_str = src.to_string_lossy().into_owned(); + let mut read_dir = match tokio::fs::read_dir(&src).await { + Ok(read_dir) => read_dir, + Err(e) => { + skipped.add(&src_str, format_args!("read local dir: {e}")); + continue; + } + }; + plan.dirs.push(Planned { + src: src_str.clone(), + dst: dst.clone(), + size: 0, + }); + loop { + let entry = match read_dir.next_entry().await { + Ok(Some(entry)) => entry, + Ok(None) => break, + Err(e) => { + skipped.add(&src_str, format_args!("read local dir: {e}")); + break; + } + }; + let path = entry.path(); + let shown = path.to_string_lossy(); + let Ok(name) = entry.file_name().into_string() else { + skipped.add(&shown, "name is not valid UTF-8"); + continue; + }; + // `DirEntry::metadata` does not follow symlinks. + let meta = match entry.metadata().await { + Ok(meta) => meta, + Err(e) => { + skipped.add(&shown, e); + continue; + } + }; + let remote = join_remote(&dst, &name); + let file_type = meta.file_type(); + if file_type.is_dir() && depth < MAX_DEPTH { + stack.push((path.clone(), remote, depth + 1)); + } else if file_type.is_dir() { + skipped.add(&shown, "nested too deep"); } else if file_type.is_file() { - let size = entry.metadata().await.map(|m| m.len()).unwrap_or(0); - plan.files.push(PlannedFile { - src: entry.path().to_string_lossy().into_owned(), - dst: join_remote(&dst, &name), - size, + plan.files.push(Planned { + src: shown.into_owned(), + dst: remote, + size: meta.len(), }); + } else if file_type.is_symlink() { + skipped.add(&shown, "symbolic link, not followed"); + } else { + skipped.add(&shown, "not a regular file or folder"); } } } Ok(plan) } -/// Walks the remote folder `root`, mapping it onto the local folder `dst_root`. -/// Symlinks and special files are skipped, as are names that cannot be created here. -async fn plan_remote_tree( - sftp: &russh_sftp::client::SftpSession, +/// Walks the remote folder `root`, mapping it onto the local folder `dst_root`; +/// `list` reads one remote folder. A symlink is skipped (servers report links, not +/// their targets), as is anything that is not a plain file or folder and any name +/// that cannot be created here. +async fn plan_remote_tree( + mut list: F, root: &str, - dst_root: &std::path::Path, -) -> anyhow::Result { + dst_root: &Path, + skipped: &mut Skipped, +) -> anyhow::Result +where + F: FnMut(String) -> Fut, + Fut: Future>>, +{ let mut plan = TreePlan::default(); - let mut stack = vec![(root.to_string(), dst_root.to_path_buf())]; - while let Some((src, dst)) = stack.pop() { - let read_dir = sftp - .read_dir(src.as_str()) - .await - .with_context(|| format!("read remote dir '{src}'"))?; - plan.dirs.push(dst.to_string_lossy().into_owned()); - for entry in read_dir { - let name = entry.file_name(); + let mut stack = vec![(root.to_string(), dst_root.to_path_buf(), 0)]; + while let Some((src, dst, depth)) = stack.pop() { + let entries = match list(src.clone()).await { + Ok(entries) => entries, + Err(e) => { + skipped.add(&src, format_args!("read remote dir: {e:#}")); + continue; + } + }; + plan.dirs.push(Planned { + src: src.clone(), + dst: dst.to_string_lossy().into_owned(), + size: 0, + }); + for (name, attrs) in entries { + let path = join_remote(&src, &name); let Some(local) = join_local(&dst, &name) else { + skipped.add(&path, "name not allowed here"); continue; }; - let file_type = entry.file_type(); - if file_type.is_dir() { - stack.push((join_remote(&src, &name), local)); - } else if file_type.is_file() { - plan.files.push(PlannedFile { - src: join_remote(&src, &name), + match attrs.file_type() { + FileType::Dir if depth < MAX_DEPTH => { + stack.push((path, local, depth + 1)); + } + FileType::Dir => skipped.add(&path, "nested too deep"), + FileType::File => plan.files.push(Planned { + src: path, dst: local.to_string_lossy().into_owned(), - size: entry.metadata().size.unwrap_or(0), - }); + size: attrs.size.unwrap_or(0), + }), + FileType::Symlink => skipped.add(&path, "symbolic link, not followed"), + FileType::Other => skipped.add(&path, "not a regular file or folder"), } } } @@ -520,30 +609,42 @@ async fn do_download( check_paths(local, remote)?; check_local_name(local)?; - // Size for progress and file-or-folder (best-effort: a failed stat downloads as a - // file, and the open below reports the real error). + // Size and type best-effort: a failed stat downloads as a file, and the open below + // reports why. Without permissions the type is unknown too. let meta = sftp.metadata(remote).await.ok(); - if meta.as_ref().is_some_and(|m| m.file_type().is_dir()) { - let plan = plan_remote_tree(sftp, remote, std::path::Path::new(local)).await?; - let mut progress = Progress::new(transfer_id, plan.total(), event_tx); - for dir in &plan.dirs { - check_paths(dir, remote)?; - tokio::fs::create_dir_all(dir) - .await - .with_context(|| format!("create local dir '{dir}'"))?; + match meta + .as_ref() + .map(|m| (m.permissions.is_some(), m.file_type())) + { + Some((_, FileType::Dir)) => {} + Some((true, FileType::Symlink | FileType::Other)) => { + anyhow::bail!("'{remote}' is not a regular file or folder") } - for file in &plan.files { - check_paths(&file.dst, &file.src)?; - download_file(sftp, &file.src, &file.dst, &mut progress) - .await - .with_context(|| format!("download '{}'", file.src))?; + _ => { + let total = meta.and_then(|m| m.size).unwrap_or(0); + let mut progress = Progress::new(transfer_id, total, event_tx); + return download_file(sftp, remote, local, &mut progress).await; } - return Ok(()); } - let total = meta.and_then(|m| m.size).unwrap_or(0); - let mut progress = Progress::new(transfer_id, total, event_tx); - download_file(sftp, remote, local, &mut progress).await + let mut skipped = Skipped::default(); + let list = |dir: String| async move { + let entries = sftp.read_dir(dir).await?; + anyhow::Ok(entries.map(|e| (e.file_name(), e.metadata())).collect()) + }; + let plan = plan_remote_tree(list, remote, Path::new(local), &mut skipped).await?; + let mut progress = Progress::new(transfer_id, plan.total(), event_tx); + for dir in &plan.dirs { + if let Err(e) = tokio::fs::create_dir_all(&dir.dst).await { + skipped.add(&dir.src, format_args!("create local dir: {e}")); + } + } + for file in &plan.files { + if let Err(e) = download_file(sftp, &file.src, &file.dst, &mut progress).await { + skipped.add(&file.src, format_args!("{e:#}")); + } + } + skipped.into_result() } async fn download_file( @@ -576,7 +677,8 @@ async fn download_file( progress.advance(n).await; } - Ok(()) + // The last write may still be in flight; this is where it fails, if it does. + local_file.flush().await.context("write local file") } /// Uploads a local file or, recursively, a local folder to `remote`. @@ -593,27 +695,46 @@ async fn do_upload( let meta = tokio::fs::metadata(local) .await .context("open local file for upload")?; - if meta.is_dir() { - let plan = plan_local_tree(std::path::Path::new(local), remote).await?; - let mut progress = Progress::new(transfer_id, plan.total(), event_tx); - for dir in &plan.dirs { - // An existing folder is merged into, as an existing file is overwritten. - if !sftp.try_exists(dir.as_str()).await.unwrap_or(false) { - sftp.create_dir(dir.as_str()) - .await - .with_context(|| format!("create remote dir '{dir}'"))?; - } + if meta.is_file() { + let mut progress = Progress::new(transfer_id, meta.len(), event_tx); + return upload_file(local, sftp, remote, &mut progress).await; + } + if !meta.is_dir() { + anyhow::bail!("'{local}' is not a regular file or folder"); + } + + let mut skipped = Skipped::default(); + let plan = plan_local_tree(local, remote, &mut skipped).await?; + let mut progress = Progress::new(transfer_id, plan.total(), event_tx); + for dir in &plan.dirs { + if let Err(e) = create_remote_dir(sftp, &dir.dst).await { + skipped.add(&dir.src, format_args!("{e:#}")); } - for file in &plan.files { - upload_file(&file.src, sftp, &file.dst, &mut progress) - .await - .with_context(|| format!("upload '{}'", file.src))?; + } + for file in &plan.files { + if let Err(e) = upload_file(&file.src, sftp, &file.dst, &mut progress).await { + skipped.add(&file.src, format_args!("{e:#}")); } - return Ok(()); } + skipped.into_result() +} - let mut progress = Progress::new(transfer_id, meta.len(), event_tx); - upload_file(local, sftp, remote, &mut progress).await +/// Creates the remote folder `path`, or merges into the one there. +async fn create_remote_dir( + sftp: &russh_sftp::client::SftpSession, + path: &str, +) -> anyhow::Result<()> { + if let Err(e) = sftp.create_dir(path).await { + if sftp + .metadata(path) + .await + .is_ok_and(|m| m.file_type().is_dir()) + { + return Ok(()); + } + return Err(e).context("create remote dir"); + } + Ok(()) } async fn upload_file( @@ -780,18 +901,20 @@ mod tests { assert!(drive_roots(0).is_empty()); } - fn scratch_dir(tag: &str) -> std::path::PathBuf { - let dir = std::env::temp_dir().join(format!("omnyssh-sftp-{tag}-{}", std::process::id())); - std::fs::create_dir_all(&dir).expect("create scratch dir"); - dir + fn attrs(permissions: u32, size: u64) -> FileAttributes { + FileAttributes { + permissions: Some(permissions), + size: Some(size), + ..FileAttributes::empty() + } } #[tokio::test] async fn local_listing_carries_modification_time() { - let dir = scratch_dir("list"); - std::fs::write(dir.join("a.txt"), b"hello").expect("write scratch file"); + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("a.txt"), b"hello").expect("write scratch file"); - let entries = list_local_dir(&dir.to_string_lossy()) + let entries = list_local_dir(&dir.path().to_string_lossy()) .await .expect("list scratch dir"); let file = entries @@ -804,28 +927,34 @@ mod tests { let parent = entries.first().expect("parent entry"); assert_eq!(parent.name, ".."); assert!(parent.modified.is_none()); - - let _ = std::fs::remove_dir_all(&dir); } #[tokio::test] - async fn local_tree_plan_maps_every_file_and_folder() { - let dir = scratch_dir("plan"); + async fn the_local_plan_maps_files_and_folders_and_reports_the_rest() { + let tmp = tempfile::tempdir().expect("tempdir"); + let dir = tmp.path(); std::fs::create_dir_all(dir.join("sub").join("deep")).expect("create scratch tree"); std::fs::write(dir.join("a.txt"), b"abc").expect("write scratch file"); std::fs::write(dir.join("sub").join("deep").join("b.bin"), b"hello") .expect("write scratch file"); #[cfg(unix)] - std::os::unix::fs::symlink(&dir, dir.join("sub").join("loop")).expect("create symlink"); + { + std::os::unix::fs::symlink(dir, dir.join("sub").join("loop")).expect("symlink"); + std::os::unix::net::UnixListener::bind(dir.join("sock")).expect("socket"); + } - let plan = plan_local_tree(&dir, "/srv/up").await.expect("plan tree"); - assert_eq!(plan.dirs[0], "/srv/up"); - let mut dirs = plan.dirs.clone(); + let mut skipped = Skipped::default(); + let root = dir.to_str().expect("utf-8 temp dir"); + let plan = plan_local_tree(root, "/srv/up", &mut skipped) + .await + .expect("plan tree"); + assert_eq!(plan.dirs[0].dst, "/srv/up"); + let mut dirs: Vec<_> = plan.dirs.iter().map(|d| d.dst.as_str()).collect(); dirs.sort(); assert_eq!(dirs, ["/srv/up", "/srv/up/sub", "/srv/up/sub/deep"]); // Parents come before their children, so creating in order never fails. - let sub = plan.dirs.iter().position(|d| d == "/srv/up/sub"); - let deep = plan.dirs.iter().position(|d| d == "/srv/up/sub/deep"); + let sub = plan.dirs.iter().position(|d| d.dst == "/srv/up/sub"); + let deep = plan.dirs.iter().position(|d| d.dst == "/srv/up/sub/deep"); assert!(sub < deep); let mut files: Vec<_> = plan .files @@ -835,8 +964,245 @@ mod tests { files.sort(); assert_eq!(files, [("/srv/up/a.txt", 3), ("/srv/up/sub/deep/b.bin", 5)]); assert_eq!(plan.total(), 8); + // The link back up and the socket are left out, and said so. + assert_eq!(skipped.count, if cfg!(unix) { 2 } else { 0 }); + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn a_local_name_that_is_not_utf8_is_reported_not_fatal() { + use std::os::unix::ffi::OsStrExt; + let tmp = tempfile::tempdir().expect("tempdir"); + std::fs::write(tmp.path().join("ok.txt"), b"ok").expect("write"); + let bad = std::ffi::OsStr::from_bytes(b"bad\xff.txt"); + std::fs::write(tmp.path().join(bad), b"bad").expect("write"); + + let mut skipped = Skipped::default(); + let root = tmp.path().to_str().expect("utf-8 temp dir"); + let plan = plan_local_tree(root, "/up", &mut skipped) + .await + .expect("plan tree"); + assert_eq!(plan.files.len(), 1); + assert_eq!(plan.files[0].dst, "/up/ok.txt"); + let err = skipped.into_result().expect_err("the bad name is reported"); + assert!( + err.to_string().ends_with("': name is not valid UTF-8"), + "{err}" + ); + } + + /// A remote file system: folder -> listing. A folder not in it cannot be read. + fn fake_server( + tree: Vec<(&'static str, Vec<(&'static str, FileAttributes)>)>, + ) -> impl FnMut(String) -> std::future::Ready>> + { + move |dir| { + let listing = tree + .iter() + .find(|(path, _)| *path == dir) + .map(|(_, entries)| { + entries + .iter() + .map(|(name, attrs)| (name.to_string(), attrs.clone())) + .collect() + }); + std::future::ready(listing.ok_or_else(|| anyhow::anyhow!("Permission denied"))) + } + } - let _ = std::fs::remove_dir_all(&dir); + #[tokio::test] + async fn the_remote_plan_maps_a_tree_and_reports_what_it_leaves_out() { + let server = fake_server(vec![ + ( + "/srv/app", + vec![ + ("run.sh", attrs(0o100_755, 3)), + ("conf", attrs(0o40_700, 0)), + ("current", attrs(0o120_777, 0)), + ("fifo", attrs(0o10_644, 0)), + ("a/b", attrs(0o100_644, 1)), + ], + ), + ( + "/srv/app/conf", + vec![ + ("app.toml", attrs(0o100_600, 5)), + ("locked", attrs(0o40_000, 0)), + ], + ), + ]); + let dst = Path::new("dl").join("app"); + let mut skipped = Skipped::default(); + let plan = plan_remote_tree(server, "/srv/app", &dst, &mut skipped) + .await + .expect("plan tree"); + + let local = |rel: &[&str]| { + rel.iter() + .fold(dst.clone(), |p, part| p.join(part)) + .to_string_lossy() + .into_owned() + }; + let dirs: Vec<_> = plan.dirs.iter().map(|d| d.dst.clone()).collect(); + assert_eq!(dirs, [local(&[]), local(&["conf"])]); + assert_eq!( + plan.files, + [ + Planned { + src: "/srv/app/run.sh".into(), + dst: local(&["run.sh"]), + size: 3, + }, + Planned { + src: "/srv/app/conf/app.toml".into(), + dst: local(&["conf", "app.toml"]), + size: 5, + }, + ] + ); + // The symlink, the FIFO, the name with a separator and the unreadable folder: + // left out, counted, and the first one named. + assert_eq!(skipped.count, 4); + let err = skipped.into_result().expect_err("skips are reported"); + assert_eq!( + err.to_string(), + "4 items skipped or failed, first '/srv/app/current': symbolic link, not followed" + ); + } + + #[tokio::test] + async fn the_remote_plan_stops_at_the_depth_cap() { + // A server that makes up one more folder at every level. + let server = |_dir: String| { + std::future::ready(anyhow::Ok(vec![("d".to_string(), attrs(0o40_755, 0))])) + }; + let mut skipped = Skipped::default(); + let plan = plan_remote_tree(server, "/x", Path::new("dl"), &mut skipped) + .await + .expect("plan tree"); + assert_eq!(plan.dirs.len(), MAX_DEPTH + 1); + let err = skipped.into_result().expect_err("the cut is reported"); + assert!(err + .to_string() + .starts_with("1 item skipped or failed, first '/x/d/d/")); + assert!(err.to_string().ends_with("/d': nested too deep"), "{err}"); + } + + #[test] + fn skips_are_counted_and_the_first_reason_kept_whole() { + assert!(Skipped::default().into_result().is_ok()); + + let mut skipped = Skipped::default(); + let cause = std::io::Error::from(std::io::ErrorKind::PermissionDenied); + let err = anyhow::Error::from(cause).context("open remote file for download"); + skipped.add("/srv/a", format_args!("{err:#}")); + let one = format!( + "1 item skipped or failed, first '/srv/a': open remote file for download: {}", + std::io::Error::from(std::io::ErrorKind::PermissionDenied) + ); + let mut again = Skipped::default(); + again.add("/srv/a", format_args!("{err:#}")); + assert_eq!(again.into_result().expect_err("one").to_string(), one); + + skipped.add("/srv/b", "symbolic link, not followed"); + assert_eq!( + skipped.into_result().expect_err("two").to_string(), + one.replacen("1 item", "2 items", 1) + ); + } + + /// An SFTP session on this machine's own files through OpenSSH's sftp-server, + /// where one is installed. + #[cfg(unix)] + async fn local_sftp() -> Option { + let server = [ + "/usr/lib/openssh/sftp-server", + "/usr/libexec/openssh/sftp-server", + "/usr/libexec/sftp-server", + ] + .into_iter() + .find(|path| Path::new(path).exists())?; + let mut child = tokio::process::Command::new(server) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .spawn() + .ok()?; + // The server exits once the session drops its end of the pipes. + let pipes = tokio::io::join(child.stdout.take()?, child.stdin.take()?); + russh_sftp::client::SftpSession::new(pipes).await.ok() + } + + #[cfg(unix)] + #[tokio::test] + async fn a_folder_round_trips_through_sftp_with_its_skips_reported() { + use std::os::unix::fs::PermissionsExt; + let Some(sftp) = local_sftp().await else { + eprintln!("no sftp-server on this machine; skipped"); + return; + }; + let tmp = tempfile::tempdir().expect("tempdir"); + let path = |rel: &str| tmp.path().join(rel); + let str_of = |p: &Path| p.to_str().expect("utf-8 temp dir").to_string(); + let set_mode = |p: PathBuf, mode| { + std::fs::set_permissions(p, std::fs::Permissions::from_mode(mode)).expect("chmod") + }; + std::fs::create_dir_all(path("src/private")).expect("mkdir"); + std::fs::write(path("src/run.sh"), "#!/bin/sh\n").expect("write"); + std::fs::write(path("src/private/data.bin"), vec![7u8; 200_000]).expect("write"); + std::os::unix::fs::symlink(path("src/run.sh"), path("src/link")).expect("symlink"); + + let (tx, _rx) = mpsc::channel(64); + let err = do_upload(&str_of(&path("src")), &sftp, &str_of(&path("up")), 1, &tx) + .await + .expect_err("the link is reported"); + let link = str_of(&path("src/link")); + assert_eq!( + err.to_string(), + format!("1 item skipped or failed, first '{link}': symbolic link, not followed") + ); + assert_eq!( + std::fs::read(path("up/private/data.bin")) + .expect("read") + .len(), + 200_000 + ); + assert!(!path("up/link").exists()); + + // Back down, into a folder that already holds one of the files. + std::fs::create_dir_all(path("down")).expect("mkdir"); + std::fs::write(path("down/run.sh"), "old").expect("write"); + do_download(&sftp, &str_of(&path("up")), &str_of(&path("down")), 2, &tx) + .await + .expect("downloaded"); + assert_eq!( + std::fs::read(path("down/run.sh")).expect("read"), + b"#!/bin/sh\n" + ); + assert_eq!( + std::fs::read(path("down/private/data.bin")) + .expect("read") + .len(), + 200_000 + ); + + // An unreadable folder is reported with the server's reason; the rest still + // arrives. Root reads it anyway, so only a plain user can check this. + set_mode(path("up/private"), 0o000); + let readable = std::fs::read_dir(path("up/private")).is_ok(); + let result = + do_download(&sftp, &str_of(&path("up")), &str_of(&path("again")), 3, &tx).await; + set_mode(path("up/private"), 0o700); + if !readable { + let err = result.expect_err("the folder is reported"); + let private = str_of(&path("up/private")); + assert!( + err.to_string().starts_with(&format!( + "1 item skipped or failed, first '{private}': read remote dir: Permission denied" + )), + "{err}" + ); + assert!(path("again/run.sh").exists()); + } } #[test] @@ -898,7 +1264,7 @@ mod tests { #[cfg(windows)] #[test] fn on_windows_a_server_name_never_leaves_the_destination() { - let dst = std::path::Path::new(r"C:\Users\me\dl"); + let dst = Path::new(r"C:\Users\me\dl"); for name in [ "C:x", "D:", "a:b", r"..\x", r"\x", r"\\?\x", "CON", "aux.c", "a.", ] { @@ -916,7 +1282,7 @@ mod tests { #[cfg(unix)] #[test] fn on_unix_backslashes_colons_and_device_names_are_plain() { - let dst = std::path::Path::new("/home/me/dl"); + let dst = Path::new("/home/me/dl"); for name in ["a\\b", "..\\x", "C:x", "a:b", "CON", "a."] { assert_eq!(join_local(dst, name), Some(dst.join(name)), "{name:?}"); } From ce7f29ee62acde5bd4cd5afc1671c58d8fa5311b Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 23:41:55 +0400 Subject: [PATCH 08/44] feat(core): cancel a running transfer --- crates/omnyssh-core/src/ssh/sftp.rs | 348 ++++++++++++++++++++++++---- 1 file changed, 308 insertions(+), 40 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index 42883ffc..8c83c524 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -8,11 +8,13 @@ use std::future::Future; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::Arc; use std::time::Duration; use anyhow::Context; use russh_sftp::protocol::{FileAttributes, FileType}; -use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt}; use tokio::sync::mpsc; use tokio::time; @@ -77,7 +79,7 @@ pub enum SftpCommand { /// path. An existing folder is merged into and an existing file replaced. Inside a /// folder, symlinks, special files and names this system cannot create are /// skipped; the transfer goes on past them and past failed files, and its - /// [`CoreEvent::SftpOpDone`] error counts them. + /// [`CoreEvent::SftpOpDone`] error counts them. [`SftpManager::cancel`] stops it. Download { remote: String, local: String, @@ -101,6 +103,9 @@ pub enum SftpCommand { Disconnect, } +/// A command and the cancel count it was sent under (see [`SftpManager::cancel`]). +type Queued = (u64, SftpCommand); + // --------------------------------------------------------------------------- // SftpManager — handle held by App to communicate with the background task // --------------------------------------------------------------------------- @@ -111,7 +116,10 @@ pub enum SftpCommand { /// commands, and [`SftpManager::disconnect`] for a clean shutdown. #[derive(Debug)] pub struct SftpManager { - cmd_tx: mpsc::Sender, + cmd_tx: mpsc::Sender, + /// How many times transfers were cancelled. Shared with the task rather than + /// queued: the transfer to stop holds the queue until it ends. + cancels: Arc, } impl SftpManager { @@ -145,34 +153,53 @@ impl SftpManager { .await .map_err(|_| anyhow::anyhow!("SFTP did not start within {}s", OPEN_TIMEOUT.as_secs()))??; - let (cmd_tx, cmd_rx) = mpsc::channel::(64); + let (cmd_tx, cmd_rx) = mpsc::channel::(64); + let cancels = Arc::new(AtomicU64::new(0)); let host_name = host.name.clone(); // `session` and `sftp` are owned by this async block. If the task // panics, Rust's unwind machinery calls their Drop impls before the // panic propagates to tokio — the TCP connection is therefore always // released even in the panic path. No explicit catch_unwind needed. + let task_cancels = Arc::clone(&cancels); tokio::spawn(async move { let _ = event_tx .send(CoreEvent::SftpConnected { host_name: host_name.clone(), }) .await; - sftp_task_loop(session, sftp, cmd_rx, event_tx.clone()).await; + sftp_task_loop(session, sftp, cmd_rx, event_tx.clone(), task_cancels).await; tracing::info!("SFTP task for '{}' exited", host_name); }); - Ok(Self { cmd_tx }) + Ok(Self { cmd_tx, cancels }) } /// Enqueues a command (fire-and-forget). Silently drops if the task exited. pub fn send(&self, cmd: SftpCommand) { - let _ = self.cmd_tx.try_send(cmd); + let _ = self + .cmd_tx + .try_send((self.cancels.load(Ordering::Relaxed), cmd)); } - /// Sends [`SftpCommand::Disconnect`] and drops the sender. + /// Cancels every transfer sent so far. The running one stops at its next step and + /// removes the file it was part way through; files it finished stay. Queued ones + /// stop before they start. Each still ends with its [`CoreEvent::SftpOpDone`], + /// whose error says it was cancelled. + pub fn cancel(&self) { + self.cancels.fetch_add(1, Ordering::Relaxed); + } + + /// Cancels the transfers, sends [`SftpCommand::Disconnect`] and drops the sender. pub fn disconnect(self) { - let _ = self.cmd_tx.try_send(SftpCommand::Disconnect); + self.send(SftpCommand::Disconnect); + } +} + +impl Drop for SftpManager { + // Nothing can follow a transfer's progress once its manager is gone. + fn drop(&mut self) { + self.cancel(); } } @@ -183,10 +210,15 @@ impl SftpManager { async fn sftp_task_loop( _ssh: SshSession, // kept alive to hold the SSH connection open sftp: russh_sftp::client::SftpSession, - mut cmd_rx: mpsc::Receiver, + mut cmd_rx: mpsc::Receiver, event_tx: mpsc::Sender, + cancels: Arc, ) { - while let Some(cmd) = cmd_rx.recv().await { + while let Some((sent, cmd)) = cmd_rx.recv().await { + let cancel = Cancel { + cancels: &cancels, + sent, + }; match cmd { SftpCommand::ListDir(path) => match do_list_dir(&sftp, &path).await { Ok(entries) => { @@ -208,7 +240,7 @@ async fn sftp_task_loop( local, transfer_id, } => { - let result = do_download(&sftp, &remote, &local, transfer_id, &event_tx) + let result = do_download(&sftp, &remote, &local, transfer_id, &event_tx, &cancel) .await .map_err(|e| format!("{e:#}")); let _ = event_tx.send(CoreEvent::SftpOpDone { result }).await; @@ -219,7 +251,7 @@ async fn sftp_task_loop( remote, transfer_id, } => { - let result = do_upload(&local, &sftp, &remote, transfer_id, &event_tx) + let result = do_upload(&local, &sftp, &remote, transfer_id, &event_tx, &cancel) .await .map_err(|e| format!("{e:#}")); let _ = event_tx.send(CoreEvent::SftpOpDone { result }).await; @@ -398,6 +430,28 @@ fn check_paths(local: &str, remote: &str) -> anyhow::Result<()> { /// up to never end. const MAX_DEPTH: usize = 64; +/// Whether the transfer at hand was cancelled: the manager's cancel count has moved +/// on from the one its command was sent under. +struct Cancel<'a> { + cancels: &'a AtomicU64, + sent: u64, +} + +impl Cancel<'_> { + fn check(&self) -> anyhow::Result<()> { + if self.cancels.load(Ordering::Relaxed) == self.sent { + Ok(()) + } else { + Err(Cancelled.into()) + } + } +} + +/// The error a cancelled transfer ends with. +#[derive(Debug, thiserror::Error)] +#[error("Transfer cancelled")] +struct Cancelled; + /// What a folder transfer left out. It goes on past every skip and failure and /// reports them in its one result. #[derive(Debug, Default)] @@ -481,11 +535,13 @@ impl<'a> Progress<'a> { async fn plan_local_tree( root: &str, dst_root: &str, + cancel: &Cancel<'_>, skipped: &mut Skipped, ) -> anyhow::Result { let mut plan = TreePlan::default(); let mut stack = vec![(PathBuf::from(root), dst_root.to_string(), 0)]; while let Some((src, dst, depth)) = stack.pop() { + cancel.check()?; // Every name on the way is UTF-8, so the path converts losslessly. let src_str = src.to_string_lossy().into_owned(); let mut read_dir = match tokio::fs::read_dir(&src).await { @@ -553,6 +609,7 @@ async fn plan_remote_tree( mut list: F, root: &str, dst_root: &Path, + cancel: &Cancel<'_>, skipped: &mut Skipped, ) -> anyhow::Result where @@ -562,6 +619,7 @@ where let mut plan = TreePlan::default(); let mut stack = vec![(root.to_string(), dst_root.to_path_buf(), 0)]; while let Some((src, dst, depth)) = stack.pop() { + cancel.check()?; let entries = match list(src.clone()).await { Ok(entries) => entries, Err(e) => { @@ -605,9 +663,11 @@ async fn do_download( local: &str, transfer_id: TransferId, event_tx: &mpsc::Sender, + cancel: &Cancel<'_>, ) -> anyhow::Result<()> { check_paths(local, remote)?; check_local_name(local)?; + cancel.check()?; // Size and type best-effort: a failed stat downloads as a file, and the open below // reports why. Without permissions the type is unknown too. @@ -623,7 +683,7 @@ async fn do_download( _ => { let total = meta.and_then(|m| m.size).unwrap_or(0); let mut progress = Progress::new(transfer_id, total, event_tx); - return download_file(sftp, remote, local, &mut progress).await; + return download_file(sftp, remote, local, &mut progress, cancel).await; } } @@ -632,16 +692,19 @@ async fn do_download( let entries = sftp.read_dir(dir).await?; anyhow::Ok(entries.map(|e| (e.file_name(), e.metadata())).collect()) }; - let plan = plan_remote_tree(list, remote, Path::new(local), &mut skipped).await?; + let plan = plan_remote_tree(list, remote, Path::new(local), cancel, &mut skipped).await?; let mut progress = Progress::new(transfer_id, plan.total(), event_tx); for dir in &plan.dirs { + cancel.check()?; if let Err(e) = tokio::fs::create_dir_all(&dir.dst).await { skipped.add(&dir.src, format_args!("create local dir: {e}")); } } for file in &plan.files { - if let Err(e) = download_file(sftp, &file.src, &file.dst, &mut progress).await { - skipped.add(&file.src, format_args!("{e:#}")); + match download_file(sftp, &file.src, &file.dst, &mut progress, cancel).await { + Err(e) if e.is::() => return Err(e), + Err(e) => skipped.add(&file.src, format_args!("{e:#}")), + Ok(()) => {} } } skipped.into_result() @@ -652,21 +715,38 @@ async fn download_file( remote: &str, local: &str, progress: &mut Progress<'_>, + cancel: &Cancel<'_>, ) -> anyhow::Result<()> { - let mut remote_file = sftp + cancel.check()?; + let remote_file = sftp .open(remote) .await .context("open remote file for download")?; + write_local(remote_file, local, progress, cancel).await +} + +/// Writes everything `source` yields to the local file `local`; a cancel removes the +/// part written so far. +async fn write_local( + mut source: impl AsyncRead + Unpin, + local: &str, + progress: &mut Progress<'_>, + cancel: &Cancel<'_>, +) -> anyhow::Result<()> { let mut local_file = tokio::fs::File::create(local) .await .context("create local file")?; let mut buf = vec![0u8; 65_536]; loop { - let n = remote_file - .read(&mut buf) - .await - .context("read remote file")?; + if let Err(e) = cancel.check() { + // tokio writes in the background: the flush lets the file close first. + let _ = local_file.flush().await; + drop(local_file); + let _ = tokio::fs::remove_file(local).await; + return Err(e); + } + let n = source.read(&mut buf).await.context("read remote file")?; if n == 0 { break; } @@ -688,8 +768,10 @@ async fn do_upload( remote: &str, transfer_id: TransferId, event_tx: &mpsc::Sender, + cancel: &Cancel<'_>, ) -> anyhow::Result<()> { check_paths(local, remote)?; + cancel.check()?; // Follows a symlink: a linked folder the user picked is uploaded as a folder. let meta = tokio::fs::metadata(local) @@ -697,23 +779,26 @@ async fn do_upload( .context("open local file for upload")?; if meta.is_file() { let mut progress = Progress::new(transfer_id, meta.len(), event_tx); - return upload_file(local, sftp, remote, &mut progress).await; + return upload_file(local, sftp, remote, &mut progress, cancel).await; } if !meta.is_dir() { anyhow::bail!("'{local}' is not a regular file or folder"); } let mut skipped = Skipped::default(); - let plan = plan_local_tree(local, remote, &mut skipped).await?; + let plan = plan_local_tree(local, remote, cancel, &mut skipped).await?; let mut progress = Progress::new(transfer_id, plan.total(), event_tx); for dir in &plan.dirs { + cancel.check()?; if let Err(e) = create_remote_dir(sftp, &dir.dst).await { skipped.add(&dir.src, format_args!("{e:#}")); } } for file in &plan.files { - if let Err(e) = upload_file(&file.src, sftp, &file.dst, &mut progress).await { - skipped.add(&file.src, format_args!("{e:#}")); + match upload_file(&file.src, sftp, &file.dst, &mut progress, cancel).await { + Err(e) if e.is::() => return Err(e), + Err(e) => skipped.add(&file.src, format_args!("{e:#}")), + Ok(()) => {} } } skipped.into_result() @@ -742,7 +827,9 @@ async fn upload_file( sftp: &russh_sftp::client::SftpSession, remote: &str, progress: &mut Progress<'_>, + cancel: &Cancel<'_>, ) -> anyhow::Result<()> { + cancel.check()?; let mut local_file = tokio::fs::File::open(local) .await .context("open local file for upload")?; @@ -753,6 +840,12 @@ async fn upload_file( let mut buf = vec![0u8; 65_536]; loop { + if let Err(e) = cancel.check() { + // Closed first, or the handle would close only after the removal. + let _ = remote_file.shutdown().await; + let _ = sftp.remove_file(remote).await; + return Err(e); + } let n = local_file.read(&mut buf).await.context("read local file")?; if n == 0 { break; @@ -901,6 +994,10 @@ mod tests { assert!(drive_roots(0).is_empty()); } + fn no_cancel(cancels: &AtomicU64) -> Cancel<'_> { + Cancel { cancels, sent: 0 } + } + fn attrs(permissions: u32, size: u64) -> FileAttributes { FileAttributes { permissions: Some(permissions), @@ -943,9 +1040,10 @@ mod tests { std::os::unix::net::UnixListener::bind(dir.join("sock")).expect("socket"); } + let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); let root = dir.to_str().expect("utf-8 temp dir"); - let plan = plan_local_tree(root, "/srv/up", &mut skipped) + let plan = plan_local_tree(root, "/srv/up", &no_cancel(&cancels), &mut skipped) .await .expect("plan tree"); assert_eq!(plan.dirs[0].dst, "/srv/up"); @@ -977,9 +1075,10 @@ mod tests { let bad = std::ffi::OsStr::from_bytes(b"bad\xff.txt"); std::fs::write(tmp.path().join(bad), b"bad").expect("write"); + let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); let root = tmp.path().to_str().expect("utf-8 temp dir"); - let plan = plan_local_tree(root, "/up", &mut skipped) + let plan = plan_local_tree(root, "/up", &no_cancel(&cancels), &mut skipped) .await .expect("plan tree"); assert_eq!(plan.files.len(), 1); @@ -1032,8 +1131,9 @@ mod tests { ), ]); let dst = Path::new("dl").join("app"); + let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); - let plan = plan_remote_tree(server, "/srv/app", &dst, &mut skipped) + let plan = plan_remote_tree(server, "/srv/app", &dst, &no_cancel(&cancels), &mut skipped) .await .expect("plan tree"); @@ -1076,10 +1176,17 @@ mod tests { let server = |_dir: String| { std::future::ready(anyhow::Ok(vec![("d".to_string(), attrs(0o40_755, 0))])) }; + let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); - let plan = plan_remote_tree(server, "/x", Path::new("dl"), &mut skipped) - .await - .expect("plan tree"); + let plan = plan_remote_tree( + server, + "/x", + Path::new("dl"), + &no_cancel(&cancels), + &mut skipped, + ) + .await + .expect("plan tree"); assert_eq!(plan.dirs.len(), MAX_DEPTH + 1); let err = skipped.into_result().expect_err("the cut is reported"); assert!(err @@ -1088,6 +1195,31 @@ mod tests { assert!(err.to_string().ends_with("/d': nested too deep"), "{err}"); } + #[tokio::test] + async fn planning_stops_at_the_next_folder_once_cancelled() { + let cancels = AtomicU64::new(0); + let server = |_dir: String| { + cancels.fetch_add(1, Ordering::Relaxed); + std::future::ready(anyhow::Ok(vec![("d".to_string(), attrs(0o40_755, 0))])) + }; + let mut skipped = Skipped::default(); + let err = plan_remote_tree( + server, + "/x", + Path::new("dl"), + &no_cancel(&cancels), + &mut skipped, + ) + .await + .expect_err("cancelled"); + assert!(err.is::()); + assert_eq!( + cancels.load(Ordering::Relaxed), + 1, + "one folder read, no more" + ); + } + #[test] fn skips_are_counted_and_the_first_reason_kept_whole() { assert!(Skipped::default().into_result().is_ok()); @@ -1111,6 +1243,80 @@ mod tests { ); } + #[test] + fn a_cancel_stops_what_was_sent_before_it_and_a_closed_manager_stops_all() { + let (cmd_tx, mut cmd_rx) = mpsc::channel(8); + let manager = SftpManager { + cmd_tx, + cancels: Arc::new(AtomicU64::new(0)), + }; + let cancels = Arc::clone(&manager.cancels); + let cancelled = |sent| { + Cancel { + cancels: &cancels, + sent, + } + .check() + .is_err_and(|e| e.is::()) + }; + + manager.send(SftpCommand::ListDir("/before".into())); + manager.cancel(); + manager.send(SftpCommand::ListDir("/after".into())); + let (before, _) = cmd_rx.try_recv().expect("queued"); + let (after, _) = cmd_rx.try_recv().expect("queued"); + assert!(cancelled(before)); + assert!(!cancelled(after)); + + manager.disconnect(); + assert!(cancelled(after)); + assert!(matches!( + cmd_rx.try_recv(), + Ok((_, SftpCommand::Disconnect)) + )); + } + + /// Hands out `data` in one read and cancels the transfer as it does. + struct CancellingReader<'a> { + data: &'a [u8], + cancels: &'a AtomicU64, + } + + impl AsyncRead for CancellingReader<'_> { + fn poll_read( + mut self: std::pin::Pin<&mut Self>, + _: &mut std::task::Context<'_>, + buf: &mut tokio::io::ReadBuf<'_>, + ) -> std::task::Poll> { + let n = self.data.len().min(buf.remaining()); + buf.put_slice(&self.data[..n]); + self.data = &self.data[n..]; + self.cancels.fetch_add(1, Ordering::Relaxed); + std::task::Poll::Ready(Ok(())) + } + } + + #[tokio::test] + async fn a_cancelled_download_removes_the_part_it_wrote() { + let tmp = tempfile::tempdir().expect("tempdir"); + let local = tmp.path().join("part.bin"); + let cancels = AtomicU64::new(0); + let (tx, _rx) = mpsc::channel(8); + let mut progress = Progress::new(1, 100, &tx); + let source = CancellingReader { + data: b"the first chunk of many", + cancels: &cancels, + }; + + let local_str = local.to_str().expect("utf-8 temp dir"); + let err = write_local(source, local_str, &mut progress, &no_cancel(&cancels)) + .await + .expect_err("cancelled"); + assert!(err.is::()); + assert_eq!(progress.done, 23, "the chunk was written before the cancel"); + assert!(!local.exists(), "the partial file is gone"); + } + /// An SFTP session on this machine's own files through OpenSSH's sftp-server, /// where one is installed. #[cfg(unix)] @@ -1151,10 +1357,18 @@ mod tests { std::fs::write(path("src/private/data.bin"), vec![7u8; 200_000]).expect("write"); std::os::unix::fs::symlink(path("src/run.sh"), path("src/link")).expect("symlink"); + let cancels = AtomicU64::new(0); let (tx, _rx) = mpsc::channel(64); - let err = do_upload(&str_of(&path("src")), &sftp, &str_of(&path("up")), 1, &tx) - .await - .expect_err("the link is reported"); + let err = do_upload( + &str_of(&path("src")), + &sftp, + &str_of(&path("up")), + 1, + &tx, + &no_cancel(&cancels), + ) + .await + .expect_err("the link is reported"); let link = str_of(&path("src/link")); assert_eq!( err.to_string(), @@ -1171,9 +1385,16 @@ mod tests { // Back down, into a folder that already holds one of the files. std::fs::create_dir_all(path("down")).expect("mkdir"); std::fs::write(path("down/run.sh"), "old").expect("write"); - do_download(&sftp, &str_of(&path("up")), &str_of(&path("down")), 2, &tx) - .await - .expect("downloaded"); + do_download( + &sftp, + &str_of(&path("up")), + &str_of(&path("down")), + 2, + &tx, + &no_cancel(&cancels), + ) + .await + .expect("downloaded"); assert_eq!( std::fs::read(path("down/run.sh")).expect("read"), b"#!/bin/sh\n" @@ -1189,8 +1410,15 @@ mod tests { // arrives. Root reads it anyway, so only a plain user can check this. set_mode(path("up/private"), 0o000); let readable = std::fs::read_dir(path("up/private")).is_ok(); - let result = - do_download(&sftp, &str_of(&path("up")), &str_of(&path("again")), 3, &tx).await; + let result = do_download( + &sftp, + &str_of(&path("up")), + &str_of(&path("again")), + 3, + &tx, + &no_cancel(&cancels), + ) + .await; set_mode(path("up/private"), 0o700); if !readable { let err = result.expect_err("the folder is reported"); @@ -1205,6 +1433,46 @@ mod tests { } } + #[cfg(unix)] + #[tokio::test] + async fn a_cancelled_upload_removes_the_partial_remote_file() { + let Some(sftp) = local_sftp().await else { + eprintln!("no sftp-server on this machine; skipped"); + return; + }; + let tmp = tempfile::tempdir().expect("tempdir"); + let src = tmp.path().join("big.bin"); + std::fs::write(&src, vec![1u8; 1_000_000]).expect("write"); + let dst = tmp.path().join("copy.bin"); + + // One slot: the upload cannot run more than a chunk ahead of this watcher, + // which cancels once the first chunk is out. + let (tx, mut rx) = mpsc::channel(1); + let cancels = AtomicU64::new(0); + let cancel = no_cancel(&cancels); + let upload = do_upload( + src.to_str().expect("utf-8"), + &sftp, + dst.to_str().expect("utf-8"), + 1, + &tx, + &cancel, + ); + let watch = async { + while let Some(CoreEvent::FileTransferProgress(_, done, _)) = rx.recv().await { + if done > 0 { + cancels.fetch_add(1, Ordering::Relaxed); + } + } + }; + let result = tokio::select! { + result = upload => result, + () = watch => unreachable!("the channel outlives the upload"), + }; + assert!(result.expect_err("cancelled").is::()); + assert!(!dst.exists(), "the partial file is gone"); + } + #[test] fn a_server_name_must_be_one_plain_component() { for name in ["report.txt", "..hidden", "with space", "caf\u{e9}.txt"] { From 6211298bc188933f447e5e1cced48476574a5ff2 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 23:43:11 +0400 Subject: [PATCH 09/44] fix(core): keep permission bits on transfer --- crates/omnyssh-core/src/ssh/sftp.rs | 248 +++++++++++++++++++++++----- 1 file changed, 203 insertions(+), 45 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index 8c83c524..0fcb3fb4 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -13,7 +13,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context; -use russh_sftp::protocol::{FileAttributes, FileType}; +use russh_sftp::protocol::{FileAttributes, FileType, OpenFlags}; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt}; use tokio::sync::mpsc; use tokio::time; @@ -76,10 +76,11 @@ pub enum SftpCommand { /// List the entries in a remote directory. ListDir(String), /// Download a remote file, or a remote folder with everything in it, to a local - /// path. An existing folder is merged into and an existing file replaced. Inside a - /// folder, symlinks, special files and names this system cannot create are - /// skipped; the transfer goes on past them and past failed files, and its - /// [`CoreEvent::SftpOpDone`] error counts them. [`SftpManager::cancel`] stops it. + /// path. An existing folder is merged into and an existing file replaced; new + /// files and folders get the source's permission bits. Inside a folder, symlinks, + /// special files and names this system cannot create are skipped; the transfer + /// goes on past them and past failed files, and its [`CoreEvent::SftpOpDone`] + /// error counts them. [`SftpManager::cancel`] stops it. Download { remote: String, local: String, @@ -482,6 +483,8 @@ struct Planned { src: String, dst: String, size: u64, + /// The source's permission bits, when known. + mode: Option, } /// Everything a folder transfer creates, worked out before the first byte moves so @@ -529,18 +532,38 @@ impl<'a> Progress<'a> { } } -/// Walks the local folder `root`, mapping it onto the remote folder `dst_root`. -/// Nothing is followed: a symlink is skipped, as is anything else that is not a -/// plain file or folder. +/// A local file's permission bits; Windows has none to give. +fn local_mode(meta: &std::fs::Metadata) -> Option { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + Some(meta.permissions().mode() & 0o777) + } + #[cfg(not(unix))] + { + let _ = meta; + None + } +} + +/// A remote entry's permission bits, when the server sent them. +fn remote_mode(attrs: &FileAttributes) -> Option { + attrs.permissions.map(|p| p & 0o777) +} + +/// Walks the local folder `root`, whose permission bits are `mode`, mapping it onto +/// the remote folder `dst_root`. Nothing is followed: a symlink is skipped, as is +/// anything else that is not a plain file or folder. async fn plan_local_tree( root: &str, + mode: Option, dst_root: &str, cancel: &Cancel<'_>, skipped: &mut Skipped, ) -> anyhow::Result { let mut plan = TreePlan::default(); - let mut stack = vec![(PathBuf::from(root), dst_root.to_string(), 0)]; - while let Some((src, dst, depth)) = stack.pop() { + let mut stack = vec![(PathBuf::from(root), dst_root.to_string(), mode, 0)]; + while let Some((src, dst, mode, depth)) = stack.pop() { cancel.check()?; // Every name on the way is UTF-8, so the path converts losslessly. let src_str = src.to_string_lossy().into_owned(); @@ -555,6 +578,7 @@ async fn plan_local_tree( src: src_str.clone(), dst: dst.clone(), size: 0, + mode, }); loop { let entry = match read_dir.next_entry().await { @@ -582,7 +606,7 @@ async fn plan_local_tree( let remote = join_remote(&dst, &name); let file_type = meta.file_type(); if file_type.is_dir() && depth < MAX_DEPTH { - stack.push((path.clone(), remote, depth + 1)); + stack.push((path.clone(), remote, local_mode(&meta), depth + 1)); } else if file_type.is_dir() { skipped.add(&shown, "nested too deep"); } else if file_type.is_file() { @@ -590,6 +614,7 @@ async fn plan_local_tree( src: shown.into_owned(), dst: remote, size: meta.len(), + mode: local_mode(&meta), }); } else if file_type.is_symlink() { skipped.add(&shown, "symbolic link, not followed"); @@ -601,13 +626,14 @@ async fn plan_local_tree( Ok(plan) } -/// Walks the remote folder `root`, mapping it onto the local folder `dst_root`; -/// `list` reads one remote folder. A symlink is skipped (servers report links, not -/// their targets), as is anything that is not a plain file or folder and any name -/// that cannot be created here. +/// Walks the remote folder `root`, whose permission bits are `mode`, mapping it onto +/// the local folder `dst_root`; `list` reads one remote folder. A symlink is skipped +/// (servers report links, not their targets), as is anything that is not a plain +/// file or folder and any name that cannot be created here. async fn plan_remote_tree( mut list: F, root: &str, + mode: Option, dst_root: &Path, cancel: &Cancel<'_>, skipped: &mut Skipped, @@ -617,8 +643,8 @@ where Fut: Future>>, { let mut plan = TreePlan::default(); - let mut stack = vec![(root.to_string(), dst_root.to_path_buf(), 0)]; - while let Some((src, dst, depth)) = stack.pop() { + let mut stack = vec![(root.to_string(), dst_root.to_path_buf(), mode, 0)]; + while let Some((src, dst, mode, depth)) = stack.pop() { cancel.check()?; let entries = match list(src.clone()).await { Ok(entries) => entries, @@ -631,6 +657,7 @@ where src: src.clone(), dst: dst.to_string_lossy().into_owned(), size: 0, + mode, }); for (name, attrs) in entries { let path = join_remote(&src, &name); @@ -640,13 +667,14 @@ where }; match attrs.file_type() { FileType::Dir if depth < MAX_DEPTH => { - stack.push((path, local, depth + 1)); + stack.push((path, local, remote_mode(&attrs), depth + 1)); } FileType::Dir => skipped.add(&path, "nested too deep"), FileType::File => plan.files.push(Planned { src: path, dst: local.to_string_lossy().into_owned(), size: attrs.size.unwrap_or(0), + mode: remote_mode(&attrs), }), FileType::Symlink => skipped.add(&path, "symbolic link, not followed"), FileType::Other => skipped.add(&path, "not a regular file or folder"), @@ -669,9 +697,10 @@ async fn do_download( check_local_name(local)?; cancel.check()?; - // Size and type best-effort: a failed stat downloads as a file, and the open below - // reports why. Without permissions the type is unknown too. + // Size, mode and type best-effort: a failed stat downloads as a file, and the open + // below reports why. Without permissions the type is unknown too. let meta = sftp.metadata(remote).await.ok(); + let mode = meta.as_ref().and_then(remote_mode); match meta .as_ref() .map(|m| (m.permissions.is_some(), m.file_type())) @@ -683,7 +712,7 @@ async fn do_download( _ => { let total = meta.and_then(|m| m.size).unwrap_or(0); let mut progress = Progress::new(transfer_id, total, event_tx); - return download_file(sftp, remote, local, &mut progress, cancel).await; + return download_file(sftp, remote, local, mode, &mut progress, cancel).await; } } @@ -692,16 +721,16 @@ async fn do_download( let entries = sftp.read_dir(dir).await?; anyhow::Ok(entries.map(|e| (e.file_name(), e.metadata())).collect()) }; - let plan = plan_remote_tree(list, remote, Path::new(local), cancel, &mut skipped).await?; + let plan = plan_remote_tree(list, remote, mode, Path::new(local), cancel, &mut skipped).await?; let mut progress = Progress::new(transfer_id, plan.total(), event_tx); for dir in &plan.dirs { cancel.check()?; - if let Err(e) = tokio::fs::create_dir_all(&dir.dst).await { + if let Err(e) = create_local_dir(&dir.dst, dir.mode).await { skipped.add(&dir.src, format_args!("create local dir: {e}")); } } for file in &plan.files { - match download_file(sftp, &file.src, &file.dst, &mut progress, cancel).await { + match download_file(sftp, &file.src, &file.dst, file.mode, &mut progress, cancel).await { Err(e) if e.is::() => return Err(e), Err(e) => skipped.add(&file.src, format_args!("{e:#}")), Ok(()) => {} @@ -714,6 +743,7 @@ async fn download_file( sftp: &russh_sftp::client::SftpSession, remote: &str, local: &str, + mode: Option, progress: &mut Progress<'_>, cancel: &Cancel<'_>, ) -> anyhow::Result<()> { @@ -722,7 +752,7 @@ async fn download_file( .open(remote) .await .context("open remote file for download")?; - write_local(remote_file, local, progress, cancel).await + write_local(remote_file, local, mode, progress, cancel).await } /// Writes everything `source` yields to the local file `local`; a cancel removes the @@ -730,10 +760,11 @@ async fn download_file( async fn write_local( mut source: impl AsyncRead + Unpin, local: &str, + mode: Option, progress: &mut Progress<'_>, cancel: &Cancel<'_>, ) -> anyhow::Result<()> { - let mut local_file = tokio::fs::File::create(local) + let mut local_file = create_local_file(local, mode) .await .context("create local file")?; @@ -761,6 +792,34 @@ async fn write_local( local_file.flush().await.context("write local file") } +/// Creates or truncates the local file `path`. A new file gets the permission bits +/// `mode` less the umask, as scp and sftp give it; an existing one keeps its own. +async fn create_local_file(path: &str, mode: Option) -> std::io::Result { + let mut options = tokio::fs::OpenOptions::new(); + options.write(true).create(true).truncate(true); + #[cfg(unix)] + if let Some(mode) = mode { + options.mode(mode); + } + #[cfg(not(unix))] + let _ = mode; + options.open(path).await +} + +/// Creates the local folder `path`, or merges into the one there. A new folder gets +/// the permission bits `mode` less the umask, owner access added so it can be filled. +async fn create_local_dir(path: &str, mode: Option) -> std::io::Result<()> { + let mut builder = tokio::fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + if let Some(mode) = mode { + builder.mode(mode | 0o700); + } + #[cfg(not(unix))] + let _ = mode; + builder.create(path).await +} + /// Uploads a local file or, recursively, a local folder to `remote`. async fn do_upload( local: &str, @@ -777,25 +836,26 @@ async fn do_upload( let meta = tokio::fs::metadata(local) .await .context("open local file for upload")?; + let mode = local_mode(&meta); if meta.is_file() { let mut progress = Progress::new(transfer_id, meta.len(), event_tx); - return upload_file(local, sftp, remote, &mut progress, cancel).await; + return upload_file(local, sftp, remote, mode, &mut progress, cancel).await; } if !meta.is_dir() { anyhow::bail!("'{local}' is not a regular file or folder"); } let mut skipped = Skipped::default(); - let plan = plan_local_tree(local, remote, cancel, &mut skipped).await?; + let plan = plan_local_tree(local, mode, remote, cancel, &mut skipped).await?; let mut progress = Progress::new(transfer_id, plan.total(), event_tx); for dir in &plan.dirs { cancel.check()?; - if let Err(e) = create_remote_dir(sftp, &dir.dst).await { + if let Err(e) = create_remote_dir(sftp, &dir.dst, dir.mode).await { skipped.add(&dir.src, format_args!("{e:#}")); } } for file in &plan.files { - match upload_file(&file.src, sftp, &file.dst, &mut progress, cancel).await { + match upload_file(&file.src, sftp, &file.dst, file.mode, &mut progress, cancel).await { Err(e) if e.is::() => return Err(e), Err(e) => skipped.add(&file.src, format_args!("{e:#}")), Ok(()) => {} @@ -804,10 +864,12 @@ async fn do_upload( skipped.into_result() } -/// Creates the remote folder `path`, or merges into the one there. +/// Creates the remote folder `path`, or merges into the one there. A new folder gets +/// the permission bits `mode`, owner access added so it can be filled. async fn create_remote_dir( sftp: &russh_sftp::client::SftpSession, path: &str, + mode: Option, ) -> anyhow::Result<()> { if let Err(e) = sftp.create_dir(path).await { if sftp @@ -819,6 +881,16 @@ async fn create_remote_dir( } return Err(e).context("create remote dir"); } + // SFTP's mkdir takes a mode, but this client sends none. + if let Some(mode) = mode { + let attrs = FileAttributes { + permissions: Some(mode | 0o700), + ..FileAttributes::empty() + }; + sftp.set_metadata(path, attrs) + .await + .context("set remote dir mode")?; + } Ok(()) } @@ -826,6 +898,7 @@ async fn upload_file( local: &str, sftp: &russh_sftp::client::SftpSession, remote: &str, + mode: Option, progress: &mut Progress<'_>, cancel: &Cancel<'_>, ) -> anyhow::Result<()> { @@ -833,8 +906,15 @@ async fn upload_file( let mut local_file = tokio::fs::File::open(local) .await .context("open local file for upload")?; + // A new file gets the source's mode less the server's umask, as sftp gives it; an + // existing one keeps its own. + let attrs = FileAttributes { + permissions: mode, + ..FileAttributes::empty() + }; + let flags = OpenFlags::CREATE | OpenFlags::TRUNCATE | OpenFlags::WRITE; let mut remote_file = sftp - .create(remote) + .open_with_flags_and_attributes(remote, flags, attrs) .await .context("create remote file for upload")?; @@ -1043,10 +1123,17 @@ mod tests { let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); let root = dir.to_str().expect("utf-8 temp dir"); - let plan = plan_local_tree(root, "/srv/up", &no_cancel(&cancels), &mut skipped) - .await - .expect("plan tree"); + let plan = plan_local_tree( + root, + Some(0o750), + "/srv/up", + &no_cancel(&cancels), + &mut skipped, + ) + .await + .expect("plan tree"); assert_eq!(plan.dirs[0].dst, "/srv/up"); + assert_eq!(plan.dirs[0].mode, Some(0o750)); let mut dirs: Vec<_> = plan.dirs.iter().map(|d| d.dst.as_str()).collect(); dirs.sort(); assert_eq!(dirs, ["/srv/up", "/srv/up/sub", "/srv/up/sub/deep"]); @@ -1057,10 +1144,17 @@ mod tests { let mut files: Vec<_> = plan .files .iter() - .map(|f| (f.dst.as_str(), f.size)) + .map(|f| (f.dst.as_str(), f.size, f.mode.is_some())) .collect(); files.sort(); - assert_eq!(files, [("/srv/up/a.txt", 3), ("/srv/up/sub/deep/b.bin", 5)]); + let moded = cfg!(unix); + assert_eq!( + files, + [ + ("/srv/up/a.txt", 3, moded), + ("/srv/up/sub/deep/b.bin", 5, moded) + ] + ); assert_eq!(plan.total(), 8); // The link back up and the socket are left out, and said so. assert_eq!(skipped.count, if cfg!(unix) { 2 } else { 0 }); @@ -1078,7 +1172,7 @@ mod tests { let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); let root = tmp.path().to_str().expect("utf-8 temp dir"); - let plan = plan_local_tree(root, "/up", &no_cancel(&cancels), &mut skipped) + let plan = plan_local_tree(root, None, "/up", &no_cancel(&cancels), &mut skipped) .await .expect("plan tree"); assert_eq!(plan.files.len(), 1); @@ -1133,9 +1227,16 @@ mod tests { let dst = Path::new("dl").join("app"); let cancels = AtomicU64::new(0); let mut skipped = Skipped::default(); - let plan = plan_remote_tree(server, "/srv/app", &dst, &no_cancel(&cancels), &mut skipped) - .await - .expect("plan tree"); + let plan = plan_remote_tree( + server, + "/srv/app", + Some(0o755), + &dst, + &no_cancel(&cancels), + &mut skipped, + ) + .await + .expect("plan tree"); let local = |rel: &[&str]| { rel.iter() @@ -1143,8 +1244,11 @@ mod tests { .to_string_lossy() .into_owned() }; - let dirs: Vec<_> = plan.dirs.iter().map(|d| d.dst.clone()).collect(); - assert_eq!(dirs, [local(&[]), local(&["conf"])]); + let dirs: Vec<_> = plan.dirs.iter().map(|d| (d.dst.clone(), d.mode)).collect(); + assert_eq!( + dirs, + [(local(&[]), Some(0o755)), (local(&["conf"]), Some(0o700))] + ); assert_eq!( plan.files, [ @@ -1152,11 +1256,13 @@ mod tests { src: "/srv/app/run.sh".into(), dst: local(&["run.sh"]), size: 3, + mode: Some(0o755), }, Planned { src: "/srv/app/conf/app.toml".into(), dst: local(&["conf", "app.toml"]), size: 5, + mode: Some(0o600), }, ] ); @@ -1181,6 +1287,7 @@ mod tests { let plan = plan_remote_tree( server, "/x", + None, Path::new("dl"), &no_cancel(&cancels), &mut skipped, @@ -1206,6 +1313,7 @@ mod tests { let err = plan_remote_tree( server, "/x", + None, Path::new("dl"), &no_cancel(&cancels), &mut skipped, @@ -1309,7 +1417,7 @@ mod tests { }; let local_str = local.to_str().expect("utf-8 temp dir"); - let err = write_local(source, local_str, &mut progress, &no_cancel(&cancels)) + let err = write_local(source, local_str, None, &mut progress, &no_cancel(&cancels)) .await .expect_err("cancelled"); assert!(err.is::()); @@ -1317,6 +1425,47 @@ mod tests { assert!(!local.exists(), "the partial file is gone"); } + #[cfg(unix)] + fn mode_of(path: &Path) -> u32 { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(path).expect("stat").permissions().mode() & 0o777 + } + + #[cfg(unix)] + #[tokio::test] + async fn a_download_keeps_the_source_mode() { + let tmp = tempfile::tempdir().expect("tempdir"); + let cancels = AtomicU64::new(0); + let (tx, _rx) = mpsc::channel(8); + let mut progress = Progress::new(1, 10, &tx); + + let script = tmp.path().join("run.sh"); + let script_str = script.to_str().expect("utf-8 temp dir"); + write_local( + &b"#!/bin/sh\n"[..], + script_str, + Some(0o755), + &mut progress, + &no_cancel(&cancels), + ) + .await + .expect("written"); + assert_eq!(std::fs::read(&script).expect("read"), b"#!/bin/sh\n"); + // The umask may take group and other bits, never the owner's. + assert_eq!(mode_of(&script) & 0o700, 0o700); + + // A read-only folder still gets the owner's access, to be filled. + let dir = tmp.path().join("ro"); + create_local_dir(dir.to_str().expect("utf-8"), Some(0o500)) + .await + .expect("created"); + assert_eq!(mode_of(&dir) & 0o700, 0o700); + // An existing folder is merged into. + create_local_dir(dir.to_str().expect("utf-8"), Some(0o755)) + .await + .expect("merged"); + } + /// An SFTP session on this machine's own files through OpenSSH's sftp-server, /// where one is installed. #[cfg(unix)] @@ -1340,7 +1489,7 @@ mod tests { #[cfg(unix)] #[tokio::test] - async fn a_folder_round_trips_through_sftp_with_its_skips_reported() { + async fn a_folder_round_trips_through_sftp_with_its_modes_and_skips_reported() { use std::os::unix::fs::PermissionsExt; let Some(sftp) = local_sftp().await else { eprintln!("no sftp-server on this machine; skipped"); @@ -1354,7 +1503,9 @@ mod tests { }; std::fs::create_dir_all(path("src/private")).expect("mkdir"); std::fs::write(path("src/run.sh"), "#!/bin/sh\n").expect("write"); + set_mode(path("src/run.sh"), 0o755); std::fs::write(path("src/private/data.bin"), vec![7u8; 200_000]).expect("write"); + set_mode(path("src/private"), 0o700); std::os::unix::fs::symlink(path("src/run.sh"), path("src/link")).expect("symlink"); let cancels = AtomicU64::new(0); @@ -1381,6 +1532,12 @@ mod tests { 200_000 ); assert!(!path("up/link").exists()); + assert_eq!( + mode_of(&path("up/run.sh")) & 0o700, + 0o700, + "still executable" + ); + assert_eq!(mode_of(&path("up/private")), 0o700, "still private"); // Back down, into a folder that already holds one of the files. std::fs::create_dir_all(path("down")).expect("mkdir"); @@ -1405,6 +1562,7 @@ mod tests { .len(), 200_000 ); + assert_eq!(mode_of(&path("down/private")), 0o700); // An unreadable folder is reported with the server's reason; the rest still // arrives. Root reads it anyway, so only a plain user can check this. From 61c834d53b43ed7cda2bddefade7deae3f61ebb9 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 23:43:27 +0400 Subject: [PATCH 10/44] feat(core): tick progress once a transfer has been planned --- crates/omnyssh-core/src/ssh/sftp.rs | 32 ++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/sftp.rs b/crates/omnyssh-core/src/ssh/sftp.rs index 0fcb3fb4..cb86f0e7 100644 --- a/crates/omnyssh-core/src/ssh/sftp.rs +++ b/crates/omnyssh-core/src/ssh/sftp.rs @@ -510,13 +510,20 @@ struct Progress<'a> { } impl<'a> Progress<'a> { - fn new(transfer_id: TransferId, total: u64, event_tx: &'a mpsc::Sender) -> Self { - Self { + /// Starts the count. Its first tick tells the frontends that planning is over. + async fn start( + transfer_id: TransferId, + total: u64, + event_tx: &'a mpsc::Sender, + ) -> Self { + let mut progress = Self { transfer_id, done: 0, total, event_tx, - } + }; + progress.advance(0).await; + progress } async fn advance(&mut self, n: usize) { @@ -711,7 +718,7 @@ async fn do_download( } _ => { let total = meta.and_then(|m| m.size).unwrap_or(0); - let mut progress = Progress::new(transfer_id, total, event_tx); + let mut progress = Progress::start(transfer_id, total, event_tx).await; return download_file(sftp, remote, local, mode, &mut progress, cancel).await; } } @@ -722,7 +729,7 @@ async fn do_download( anyhow::Ok(entries.map(|e| (e.file_name(), e.metadata())).collect()) }; let plan = plan_remote_tree(list, remote, mode, Path::new(local), cancel, &mut skipped).await?; - let mut progress = Progress::new(transfer_id, plan.total(), event_tx); + let mut progress = Progress::start(transfer_id, plan.total(), event_tx).await; for dir in &plan.dirs { cancel.check()?; if let Err(e) = create_local_dir(&dir.dst, dir.mode).await { @@ -838,7 +845,7 @@ async fn do_upload( .context("open local file for upload")?; let mode = local_mode(&meta); if meta.is_file() { - let mut progress = Progress::new(transfer_id, meta.len(), event_tx); + let mut progress = Progress::start(transfer_id, meta.len(), event_tx).await; return upload_file(local, sftp, remote, mode, &mut progress, cancel).await; } if !meta.is_dir() { @@ -847,7 +854,7 @@ async fn do_upload( let mut skipped = Skipped::default(); let plan = plan_local_tree(local, mode, remote, cancel, &mut skipped).await?; - let mut progress = Progress::new(transfer_id, plan.total(), event_tx); + let mut progress = Progress::start(transfer_id, plan.total(), event_tx).await; for dir in &plan.dirs { cancel.check()?; if let Err(e) = create_remote_dir(sftp, &dir.dst, dir.mode).await { @@ -1410,7 +1417,7 @@ mod tests { let local = tmp.path().join("part.bin"); let cancels = AtomicU64::new(0); let (tx, _rx) = mpsc::channel(8); - let mut progress = Progress::new(1, 100, &tx); + let mut progress = Progress::start(1, 100, &tx).await; let source = CancellingReader { data: b"the first chunk of many", cancels: &cancels, @@ -1437,7 +1444,7 @@ mod tests { let tmp = tempfile::tempdir().expect("tempdir"); let cancels = AtomicU64::new(0); let (tx, _rx) = mpsc::channel(8); - let mut progress = Progress::new(1, 10, &tx); + let mut progress = Progress::start(1, 10, &tx).await; let script = tmp.path().join("run.sh"); let script_str = script.to_str().expect("utf-8 temp dir"); @@ -1509,7 +1516,7 @@ mod tests { std::os::unix::fs::symlink(path("src/run.sh"), path("src/link")).expect("symlink"); let cancels = AtomicU64::new(0); - let (tx, _rx) = mpsc::channel(64); + let (tx, mut rx) = mpsc::channel(64); let err = do_upload( &str_of(&path("src")), &sftp, @@ -1538,6 +1545,11 @@ mod tests { "still executable" ); assert_eq!(mode_of(&path("up/private")), 0o700, "still private"); + // The first tick (nothing done yet) marks the end of planning. + assert!(matches!( + rx.try_recv(), + Ok(CoreEvent::FileTransferProgress(1, 0, 200_010)) + )); // Back down, into a folder that already holds one of the files. std::fs::create_dir_all(path("down")).expect("mkdir"); From 6f6a502d128032813052fa1806f279cbd73eda47 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 23:48:43 +0400 Subject: [PATCH 11/44] docs(gui): say that transfers take folders --- crates/omnyssh-gui/src/commands/sftp.rs | 10 ++++++---- crates/omnyssh-gui/ui/src/lib/bindings.ts | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/omnyssh-gui/src/commands/sftp.rs b/crates/omnyssh-gui/src/commands/sftp.rs index ab6d971d..2a85d4de 100644 --- a/crates/omnyssh-gui/src/commands/sftp.rs +++ b/crates/omnyssh-gui/src/commands/sftp.rs @@ -72,8 +72,9 @@ pub fn sftp_list( Ok(()) } -/// Upload a local file to a remote path (tech-gui.md §4.2). Allocates a transfer id -/// owned by this session so `transfer-progress` routes back to the tab (§3.4). +/// Upload a local file or folder to a remote path (tech-gui.md §4.2). Allocates a +/// transfer id owned by this session so `transfer-progress` routes back to the tab +/// (§3.4). #[tauri::command] #[specta::specta] pub fn sftp_upload( @@ -94,8 +95,9 @@ pub fn sftp_upload( Ok(()) } -/// Download a remote file to a local path (tech-gui.md §4.2). See `sftp_upload` for -/// the transfer-id routing; the core guards the local destination against `..` (§3.2). +/// Download a remote file or folder to a local path (tech-gui.md §4.2). See +/// `sftp_upload` for the transfer-id routing; the core keeps every name the server +/// gives inside the local destination (§3.2). #[tauri::command] #[specta::specta] pub fn sftp_download( diff --git a/crates/omnyssh-gui/ui/src/lib/bindings.ts b/crates/omnyssh-gui/ui/src/lib/bindings.ts index f6ab9490..694b6a5b 100644 --- a/crates/omnyssh-gui/ui/src/lib/bindings.ts +++ b/crates/omnyssh-gui/ui/src/lib/bindings.ts @@ -196,8 +196,9 @@ async sftpList(sessionId: number, path: string) : Promise> { try { @@ -208,8 +209,9 @@ async sftpUpload(sessionId: number, local: string, remote: string) : Promise> { try { From 49c1e46647706cf2a9b966413703187aebf32b2e Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Thu, 1 Oct 2026 23:48:57 +0400 Subject: [PATCH 12/44] feat(gui): cancel a tab's transfer with sftp_cancel --- crates/omnyssh-gui/src/commands/sftp.rs | 11 ++++++++++- crates/omnyssh-gui/src/main.rs | 5 +++-- crates/omnyssh-gui/src/state.rs | 16 ++++++++++++++-- crates/omnyssh-gui/ui/src/lib/bindings.ts | 14 +++++++++++++- 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/crates/omnyssh-gui/src/commands/sftp.rs b/crates/omnyssh-gui/src/commands/sftp.rs index 2a85d4de..02cf29ad 100644 --- a/crates/omnyssh-gui/src/commands/sftp.rs +++ b/crates/omnyssh-gui/src/commands/sftp.rs @@ -167,7 +167,16 @@ pub fn sftp_preview( Ok(()) } -/// Close an SFTP session and its connection (tech-gui.md §4.2). +/// Cancel the transfer this session is running (tech-gui.md §4.2). It stops at its +/// next step, and its `sftp-op-done` says it was cancelled. +#[tauri::command] +#[specta::specta] +pub fn sftp_cancel(state: State<'_, GuiState>, session_id: u64) -> Result<(), CommandError> { + state.cancel_sftp(session_id); + Ok(()) +} + +/// Close an SFTP session and its connection, stopping its transfer (tech-gui.md §4.2). #[tauri::command] #[specta::specta] pub fn sftp_close(state: State<'_, GuiState>, session_id: u64) -> Result<(), CommandError> { diff --git a/crates/omnyssh-gui/src/main.rs b/crates/omnyssh-gui/src/main.rs index 0148f6c9..e9ad0c92 100644 --- a/crates/omnyssh-gui/src/main.rs +++ b/crates/omnyssh-gui/src/main.rs @@ -18,8 +18,8 @@ use commands::auth::{answer_password, unlock_identity}; use commands::hosts::{delete_host, list_hosts, refresh_metrics, reload_hosts, save_host}; use commands::keysetup::start_key_setup; use commands::sftp::{ - list_local_dir, list_local_roots, preview_local_file, sftp_close, sftp_delete, sftp_download, - sftp_list, sftp_mkdir, sftp_open, sftp_preview, sftp_rename, sftp_upload, + list_local_dir, list_local_roots, preview_local_file, sftp_cancel, sftp_close, sftp_delete, + sftp_download, sftp_list, sftp_mkdir, sftp_open, sftp_preview, sftp_rename, sftp_upload, }; use commands::snippets::{delete_snippet, execute_snippet, list_snippets, save_snippet}; use commands::terminal::{ @@ -112,6 +112,7 @@ fn specta_builder() -> Builder { sftp_rename, sftp_delete, sftp_preview, + sftp_cancel, sftp_close, list_local_dir, list_local_roots, diff --git a/crates/omnyssh-gui/src/state.rs b/crates/omnyssh-gui/src/state.rs index 5aad0516..10911170 100644 --- a/crates/omnyssh-gui/src/state.rs +++ b/crates/omnyssh-gui/src/state.rs @@ -458,6 +458,18 @@ impl GuiState { } } + /// Cancel the transfer a live SFTP session is running; unknown ids are a no-op. + pub fn cancel_sftp(&self, session_id: SessionId) { + if let Some(manager) = self + .sftp + .lock() + .expect("sftp lock poisoned") + .get(&session_id) + { + manager.cancel(); + } + } + /// Allocate a transfer id owned by `session_id`, so its `FileTransferProgress` /// events route back to the right tab via `transfer_owner` (§3.4). pub fn next_transfer(&self, session_id: SessionId) -> TransferId { @@ -478,8 +490,8 @@ impl GuiState { .copied() } - /// User-initiated SFTP close: drop the manager (a graceful `Disconnect` to its - /// task) and prune this session's transfer-owner entries (§3.4). + /// User-initiated SFTP close: drop the manager (its transfer cancelled, a graceful + /// `Disconnect` to its task) and prune this session's transfer-owner entries (§3.4). pub fn close_sftp(&self, session_id: SessionId) { if let Some(manager) = self .sftp diff --git a/crates/omnyssh-gui/ui/src/lib/bindings.ts b/crates/omnyssh-gui/ui/src/lib/bindings.ts index 694b6a5b..0dd760a7 100644 --- a/crates/omnyssh-gui/ui/src/lib/bindings.ts +++ b/crates/omnyssh-gui/ui/src/lib/bindings.ts @@ -266,7 +266,19 @@ async sftpPreview(sessionId: number, path: string) : Promise> { + try { + return { status: "ok", data: await TAURI_INVOKE("sftp_cancel", { sessionId }) }; +} catch (e) { + if(e instanceof Error) throw e; + else return { status: "error", error: e as any }; +} +}, +/** + * Close an SFTP session and its connection, stopping its transfer (tech-gui.md §4.2). */ async sftpClose(sessionId: number) : Promise> { try { From dae9c3f35ac6df6ea4fad70f12c484d9f1a8e305 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:00:27 +0400 Subject: [PATCH 13/44] docs(gui): the transfer wrappers take folders too --- crates/omnyssh-gui/ui/src/lib/ipc/commands.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts index cf40818d..42094694 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts @@ -112,13 +112,14 @@ export async function sftpList(sessionId: number, path: string): Promise { if (res.status === 'error') throw new Error(res.error.message); } -/** Upload a local file to a remote path; progress arrives as `transfer-progress`. */ +/** Upload a local file or folder to a remote path; progress arrives as `transfer-progress`. */ export async function sftpUpload(sessionId: number, local: string, remote: string): Promise { const res = await commands.sftpUpload(sessionId, local, remote); if (res.status === 'error') throw new Error(res.error.message); } -/** Download a remote file to a local path; progress arrives as `transfer-progress`. */ +/** Download a remote file or folder to a local path; progress arrives as + * `transfer-progress`. */ export async function sftpDownload( sessionId: number, local: string, From 7c8088e46f11c7e1403e83cdfa8a16b186f7a423 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:05:49 +0400 Subject: [PATCH 14/44] feat(gui): cancel a transfer from its progress strip --- crates/omnyssh-gui/ui/e2e/sftp.spec.ts | 66 ++++++++++++--- crates/omnyssh-gui/ui/src/lib/ipc/commands.ts | 6 ++ .../ui/src/lib/screens/SftpView.svelte | 80 +++++++++++++------ crates/omnyssh-gui/ui/src/lib/stores/sftp.ts | 3 + 4 files changed, 123 insertions(+), 32 deletions(-) diff --git a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts index 20f6d938..b18e1609 100644 --- a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts +++ b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts @@ -23,7 +23,7 @@ async function boot(page: Page, windows = false): Promise { let nextTransfer = 0; // A pending transfer holds until the test fires its op-done, so the progress bar // is observable mid-flight (the core is sequential — one transfer at a time). - const completions: Array<() => void> = []; + const completions: Array<{ sessionId: number; finish: () => void }> = []; type Entry = { name: string; @@ -53,11 +53,13 @@ async function boot(page: Page, windows = false): Promise { { name: 'work', path: '/home/user/work', size: 0, isDir: true } ] }; - // Where each transfer was sent, for the test to read back. + // Where each transfer was sent, and which sessions cancelled, for the test to read back. const downloads: string[] = []; const uploads: string[] = []; + const cancels: number[] = []; (win as { __downloads?: string[] }).__downloads = downloads; (win as { __uploads?: string[] }).__uploads = uploads; + (win as { __cancels?: number[] }).__cancels = cancels; (win as { __fireEvent?: unknown }).__fireEvent = (event: string, payload: unknown) => fireEvent(event, payload); const remote: Record = { @@ -93,7 +95,8 @@ async function boot(page: Page, windows = false): Promise { } // Fire the oldest still-pending transfer's op-done (deterministic completion). - (win as { __completeTransfer?: () => void }).__completeTransfer = () => completions.shift()?.(); + (win as { __completeTransfer?: () => void }).__completeTransfer = () => + completions.shift()?.finish(); (win as { __TAURI_INTERNALS__: unknown }).__TAURI_INTERNALS__ = { // `getCurrentWebview()` reads these to scope OS drag-drop listeners. @@ -138,9 +141,12 @@ async function boot(page: Page, windows = false): Promise { uploads.push(dest); const tid = ++nextTransfer; setTimeout(() => fireEvent('transfer-progress', { sessionId, transferId: tid, done: 4, total: 8 }), 0); - completions.push(() => { - addFile(remote, parentOf(dest), baseName(dest)); - fireEvent('sftp-op-done', { sessionId, ok: true }); + completions.push({ + sessionId, + finish: () => { + addFile(remote, parentOf(dest), baseName(dest)); + fireEvent('sftp-op-done', { sessionId, ok: true }); + } }); return Promise.resolve(null); } @@ -149,12 +155,29 @@ async function boot(page: Page, windows = false): Promise { downloads.push(dest); const tid = ++nextTransfer; setTimeout(() => fireEvent('transfer-progress', { sessionId, transferId: tid, done: 2, total: 8 }), 0); - completions.push(() => { - addFile(local, parentOf(dest), baseName(dest)); - fireEvent('sftp-op-done', { sessionId, ok: true }); + completions.push({ + sessionId, + finish: () => { + addFile(local, parentOf(dest), baseName(dest)); + fireEvent('sftp-op-done', { sessionId, ok: true }); + } }); return Promise.resolve(null); } + case 'sftp_cancel': { + // As the core does: the session's running transfer stops and says so. + const { sessionId } = args as { sessionId: number }; + cancels.push(sessionId); + const i = completions.findIndex((c) => c.sessionId === sessionId); + if (i >= 0) { + completions.splice(i, 1); + setTimeout( + () => fireEvent('sftp-op-done', { sessionId, ok: false, error: 'Transfer cancelled' }), + 0 + ); + } + return Promise.resolve(null); + } case 'sftp_close': return Promise.resolve(null); case 'plugin:event|listen': { @@ -227,6 +250,31 @@ test('round-trip: upload a local file to the remote, then download a remote file await expect(localPane.getByText('config.yml')).toBeVisible(); }); +test('Cancel stops the running transfer and drops the queued ones', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('notes.txt')).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + // A file, then a folder queued behind it. + await localPane.getByRole('checkbox', { name: 'Mark notes.txt' }).click(); + await localPane.getByRole('checkbox', { name: 'Mark work' }).click(); + await page.getByRole('button', { name: 'Upload' }).click(); + const strip = page.getByLabel('transfer progress'); + await expect(strip).toBeVisible(); + + await strip.getByRole('button', { name: 'Cancel transfer' }).click(); + await expect(strip).toHaveCount(0); + await expect(page.getByText('Transfer cancelled')).toBeVisible(); + const sent = await page.evaluate(() => { + const w = window as unknown as { __uploads: string[]; __cancels: number[] }; + return { uploads: w.__uploads, cancels: w.__cancels }; + }); + expect(sent).toEqual({ uploads: ['/notes.txt'], cancels: [1] }); +}); + test('the panes show modification times', async ({ page }) => { await boot(page); await page.getByTitle('files on web-1').click(); diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts index 42094694..89a73e81 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/commands.ts @@ -153,6 +153,12 @@ export async function sftpPreview(sessionId: number, path: string): Promise { + const res = await commands.sftpCancel(sessionId); + if (res.status === 'error') throw new Error(res.error.message); +} + /** Close an SFTP session and its connection. Idempotent for an already-closed id. */ export async function sftpClose(sessionId: number): Promise { const res = await commands.sftpClose(sessionId); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index b1a11173..234bbdf2 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -22,6 +22,7 @@ formatBytes, rootOf, baseName, + CANCELLED, type PaneSide } from '$lib/stores/sftp'; import { lastError } from '$lib/stores/notifications'; @@ -35,6 +36,7 @@ sftpRename, sftpDelete, sftpPreview, + sftpCancel, listLocalDir, listLocalRoots, previewLocalFile @@ -50,8 +52,9 @@ // Queued mutations, dispatched one at a time (see the pump effect). The core's SFTP // command channel is bounded and drops on overflow, so a large batch fired at once // would silently lose commands and wedge the op-done FIFO; gating on the previous - // op's completion keeps at most one command outstanding. - let outbox = $state void>>([]); + // op's completion keeps at most one command outstanding. Transfers are flagged so a + // cancel drops the queued ones too. + let outbox = $state void }>>([]); // A pending mkdir/rename input. Rename carries the entry being renamed. let prompt = $state<{ kind: 'mkdir' | 'rename'; value: string; target?: FileEntryDto } | null>( @@ -180,7 +183,7 @@ if (!view || view.pending.length > 0 || outbox.length === 0) return; const [next, ...rest] = outbox; outbox = rest; - next(); + next.run(); }); // Re-list the affected pane once every queued mutation has drained — the FS changed @@ -227,14 +230,23 @@ }; } - function enqueue(...actions: Array<() => void>): void { + function enqueue(actions: Array<() => void>, transfer = false): void { if (!actions.length) return; // Clear the prior batch's lingering error only when starting from idle. Piling onto a // batch that is still draining must not wipe a failure it already recorded (that error // stays visible until the next fresh action — see applyOpDone). const draining = outbox.length > 0 || (view?.pending.length ?? 0) > 0; if (backendId != null && !draining) sftp.clearError(backendId); - outbox = [...outbox, ...actions]; + outbox = [...outbox, ...actions.map((run) => ({ transfer, run }))]; + } + + // Stops the running transfer and drops the queued ones; the core cancels what was + // sent before this, so a transfer started afterwards runs. + function cancelTransfers(): void { + const id = backendId; + if (id == null) return; + outbox = outbox.filter((op) => !op.transfer); + void sftpCancel(id).catch((err) => lastError.set(errMsg(err))); } // `dir` defaults to the other pane's current directory; a drop onto a folder row @@ -243,10 +255,11 @@ const id = backendId; if (id == null || !view || dir == null) return; enqueue( - ...files.map((file) => () => { + files.map((file) => () => { sftp.pushOp(id, { kind: 'upload', name: file.name, refresh: 'remote' }); void sftpUpload(id, file.path, joinRemote(dir, file.name)).catch(onDispatchError(id)); - }) + }), + true ); } @@ -254,10 +267,11 @@ const id = backendId; if (id == null || !view || dir == null) return; enqueue( - ...files.map((file) => () => { + files.map((file) => () => { sftp.pushOp(id, { kind: 'download', name: file.name, refresh: 'local' }); void sftpDownload(id, joinLocal(dir, file.name), file.path).catch(onDispatchError(id)); - }) + }), + true ); } @@ -401,7 +415,7 @@ const id = backendId; if (id == null) return; enqueue( - ...remoteMarked.map((entry) => () => { + remoteMarked.map((entry) => () => { sftp.pushOp(id, { kind: 'delete', name: entry.name, refresh: 'remote' }); void sftpDelete(id, entry.path).catch(onDispatchError(id)); }) @@ -423,16 +437,20 @@ if (!value) return; const dir = view.remote.path; if (prompt.kind === 'mkdir') { - enqueue(() => { - sftp.pushOp(id, { kind: 'mkdir', refresh: 'remote' }); - void sftpMkdir(id, joinRemote(dir, value)).catch(onDispatchError(id)); - }); + enqueue([ + () => { + sftp.pushOp(id, { kind: 'mkdir', refresh: 'remote' }); + void sftpMkdir(id, joinRemote(dir, value)).catch(onDispatchError(id)); + } + ]); } else if (prompt.target) { const from = prompt.target.path; - enqueue(() => { - sftp.pushOp(id, { kind: 'rename', refresh: 'remote' }); - void sftpRename(id, from, joinRemote(dir, value)).catch(onDispatchError(id)); - }); + enqueue([ + () => { + sftp.pushOp(id, { kind: 'rename', refresh: 'remote' }); + void sftpRename(id, from, joinRemote(dir, value)).catch(onDispatchError(id)); + } + ]); } prompt = null; } @@ -620,10 +638,22 @@ {transfer.kind === 'upload' ? 'Uploading' : 'Downloading'} {transfer.name} - - {formatBytes(transfer.done)}{transfer.total > 0 - ? ` / ${formatBytes(transfer.total)}` - : ''} + + + {formatBytes(transfer.done)}{transfer.total > 0 + ? ` / ${formatBytes(transfer.total)}` + : ''} + +
    @@ -634,7 +664,11 @@
    {:else if view.error} -
    + +
    {view.error}
    {/if} diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts index 61f6ff58..3a8b91fe 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts @@ -34,6 +34,9 @@ interface Preview { content: string; } +/** The `sftp-op-done` error of a cancelled transfer, in the core's words. */ +export const CANCELLED = 'Transfer cancelled'; + // A mutating op awaiting its `sftp-op-done`. The core processes commands sequentially, // so op-done events arrive in issue order — this FIFO correlates each op-done to the op // that produced it (the contract carries no op id, §4.3). `refresh` is the pane whose From 1fa99428b52ee73c9da3fd1301ba8801393ef9cf Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:08:01 +0400 Subject: [PATCH 15/44] feat(gui): show a transfer as preparing until its first tick --- crates/omnyssh-gui/ui/e2e/sftp.spec.ts | 65 ++++++++++++++++--- .../ui/src/lib/screens/SftpView.svelte | 36 ++++++---- .../ui/src/lib/stores/sftp.test.ts | 30 +++++++++ crates/omnyssh-gui/ui/src/lib/stores/sftp.ts | 12 ++++ 4 files changed, 122 insertions(+), 21 deletions(-) diff --git a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts index b18e1609..7536978f 100644 --- a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts +++ b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts @@ -13,9 +13,14 @@ const HOSTS = [ ]; // `windows` swaps in a Windows-shaped local side: a home on C: and a second drive, D:. -async function boot(page: Page, windows = false): Promise { +// `holdProgress` keeps a transfer's first tick back until `__tick()`, as while the core +// walks a folder. +async function boot( + page: Page, + { windows = false, holdProgress = false }: { windows?: boolean; holdProgress?: boolean } = {} +): Promise { await page.addInitScript( - ({ hosts, windows }) => { + ({ hosts, windows, holdProgress }) => { let cbid = 0; const win = window as unknown as Record; const listeners: Record = {}; @@ -23,7 +28,7 @@ async function boot(page: Page, windows = false): Promise { let nextTransfer = 0; // A pending transfer holds until the test fires its op-done, so the progress bar // is observable mid-flight (the core is sequential — one transfer at a time). - const completions: Array<{ sessionId: number; finish: () => void }> = []; + const completions: Array<{ sessionId: number; tid: number; finish: () => void }> = []; type Entry = { name: string; @@ -97,6 +102,13 @@ async function boot(page: Page, windows = false): Promise { // Fire the oldest still-pending transfer's op-done (deterministic completion). (win as { __completeTransfer?: () => void }).__completeTransfer = () => completions.shift()?.finish(); + // A progress tick for the oldest pending transfer. + function tick(done: number, total: number): void { + const c = completions[0]; + if (!c) return; + fireEvent('transfer-progress', { sessionId: c.sessionId, transferId: c.tid, done, total }); + } + (win as { __tick?: typeof tick }).__tick = tick; (win as { __TAURI_INTERNALS__: unknown }).__TAURI_INTERNALS__ = { // `getCurrentWebview()` reads these to scope OS drag-drop listeners. @@ -140,9 +152,10 @@ async function boot(page: Page, windows = false): Promise { const { sessionId, remote: dest } = args as { sessionId: number; remote: string }; uploads.push(dest); const tid = ++nextTransfer; - setTimeout(() => fireEvent('transfer-progress', { sessionId, transferId: tid, done: 4, total: 8 }), 0); + if (!holdProgress) setTimeout(() => tick(4, 8), 0); completions.push({ sessionId, + tid, finish: () => { addFile(remote, parentOf(dest), baseName(dest)); fireEvent('sftp-op-done', { sessionId, ok: true }); @@ -154,9 +167,10 @@ async function boot(page: Page, windows = false): Promise { const { sessionId, local: dest } = args as { sessionId: number; local: string }; downloads.push(dest); const tid = ++nextTransfer; - setTimeout(() => fireEvent('transfer-progress', { sessionId, transferId: tid, done: 2, total: 8 }), 0); + if (!holdProgress) setTimeout(() => tick(2, 8), 0); completions.push({ sessionId, + tid, finish: () => { addFile(local, parentOf(dest), baseName(dest)); fireEvent('sftp-op-done', { sessionId, ok: true }); @@ -199,13 +213,18 @@ async function boot(page: Page, windows = false): Promise { } }; }, - { hosts: HOSTS, windows } + { hosts: HOSTS, windows, holdProgress } ); await page.goto('/'); await expect(page.getByText('2 hosts')).toBeVisible(); } +// Fire the oldest pending transfer's op-done. +async function complete(page: Page): Promise { + await page.evaluate(() => (window as unknown as { __completeTransfer: () => void }).__completeTransfer()); +} + test('host-first: a card’s files opens SFTP and browses both sides', async ({ page }) => { await boot(page); @@ -275,6 +294,34 @@ test('Cancel stops the running transfer and drops the queued ones', async ({ pag expect(sent).toEqual({ uploads: ['/notes.txt'], cancels: [1] }); }); +test('a transfer shows as preparing until its first tick, a folder of empty files too', async ({ + page +}) => { + await boot(page, { holdProgress: true }); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('work')).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + await localPane.getByRole('checkbox', { name: 'Mark work' }).click(); + await page.getByRole('button', { name: 'Upload' }).click(); + const strip = page.getByLabel('transfer progress'); + await expect(strip).toContainText('Preparing work…'); + await expect(strip.getByRole('button', { name: 'Cancel transfer' })).toBeVisible(); + + // Planned, and nothing in it has a byte: the core's first tick is 0 of 0. + await page.evaluate(() => { + (window as unknown as { __tick: (done: number, total: number) => void }).__tick(0, 0); + }); + await expect(strip).toContainText('Uploading work'); + await expect(strip).toContainText('0 B'); + + await complete(page); + await expect(strip).toHaveCount(0); + await expect(remotePane.getByText('work')).toBeVisible(); +}); + test('the panes show modification times', async ({ page }) => { await boot(page); await page.getByTitle('files on web-1').click(); @@ -425,7 +472,7 @@ test('action-first: the SFTP spawner opens the host picker, then a live session' test('Windows: the local pane switches drives, and downloads land on the chosen one', async ({ page }) => { - await boot(page, true); + await boot(page, { windows: true }); await page.getByTitle('files on web-1').click(); const localPane = page.getByRole('region', { name: 'Local' }); @@ -448,7 +495,7 @@ test('Windows: the local pane switches drives, and downloads land on the chosen test('Windows: a drive that fails to list says why, and the selector stays on the pane’s drive', async ({ page }) => { - await boot(page, true); + await boot(page, { windows: true }); await page.getByTitle('files on web-1').click(); const localPane = page.getByRole('region', { name: 'Local' }); const drive = localPane.getByRole('combobox', { name: 'Local drive' }); @@ -472,7 +519,7 @@ test('a single-root system shows no drive switch', async ({ page }) => { }); test('Windows: a slow drive left for another does not take the pane back', async ({ page }) => { - await boot(page, true); + await boot(page, { windows: true }); await page.getByTitle('files on web-1').click(); const localPane = page.getByRole('region', { name: 'Local' }); const drive = localPane.getByRole('combobox', { name: 'Local drive' }); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index 234bbdf2..116b3c6a 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -22,6 +22,7 @@ formatBytes, rootOf, baseName, + transferState, CANCELLED, type PaneSide } from '$lib/stores/sftp'; @@ -78,7 +79,7 @@ await refreshLocal(root); selectedDrive = drive; } - const transfer = $derived(view?.transfer); + const transfer = $derived(view ? transferState(view) : undefined); // Folders transfer whole, so the Upload/Download buttons take them like files. const localMarkedItems = $derived(view ? markedTransferable(view.local) : []); @@ -635,15 +636,21 @@
    - {transfer.kind === 'upload' ? 'Uploading' : 'Downloading'} - {transfer.name} + {transfer.preparing + ? 'Preparing' + : transfer.kind === 'upload' + ? 'Uploading' + : 'Downloading'} + {transfer.name}{transfer.preparing ? '…' : ''} - - {formatBytes(transfer.done)}{transfer.total > 0 - ? ` / ${formatBytes(transfer.total)}` - : ''} - + {#if !transfer.preparing} + + {formatBytes(transfer.done)}{transfer.total > 0 + ? ` / ${formatBytes(transfer.total)}` + : ''} + + {/if}
    -
    + {#if transfer.preparing} + +
    + {:else} +
    + {/if}
    {:else if view.error} diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts index 56821991..64d40cdf 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.test.ts @@ -10,6 +10,7 @@ import { mergeRefresh, applyProgress, applyOpDone, + transferState, formatBytes, formatDate, dragPayload, @@ -189,6 +190,35 @@ describe('sftp reducers', () => { expect(s.refresh).toBe('remote'); }); + it('transferState shows a dispatched transfer as preparing until its first tick', () => { + const idle = newSession('web-1'); + expect(transferState(idle)).toBeUndefined(); + const mkdir: SftpSession = { ...idle, pending: [{ kind: 'mkdir', refresh: 'remote' }] }; + expect(transferState(mkdir)).toBeUndefined(); + + let s: SftpSession = { + ...idle, + pending: [{ kind: 'download', name: 'logs', refresh: 'local' }] + }; + expect(transferState(s)).toEqual({ + kind: 'download', + name: 'logs', + done: 0, + total: 0, + preparing: true + }); + // A folder of empty files still ticks once planned: 0 of 0, no longer preparing. + s = applyProgress(s, { sessionId: 1, transferId: 3, done: 0, total: 0 }); + expect(transferState(s)).toEqual({ + kind: 'download', + name: 'logs', + done: 0, + total: 0, + preparing: false + }); + expect(transferState(applyOpDone(s, true))).toBeUndefined(); + }); + it('formatBytes is human readable', () => { expect(formatBytes(512)).toBe('512 B'); expect(formatBytes(2048)).toBe('2.0 KB'); diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts index 3a8b91fe..f8b7b9cd 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts @@ -180,6 +180,18 @@ export function applyProgress(session: SftpSession, p: TransferProgressDto): Sft }; } +/** What the transfer strip shows: the running transfer's last tick or, from its dispatch + * until the first tick (the core may spend a while walking a folder), that it is being + * prepared. */ +export function transferState( + session: SftpSession +): (Transfer & { preparing: boolean }) | undefined { + if (session.transfer) return { ...session.transfer, preparing: false }; + const front = session.pending[0]; + if (front?.kind !== 'upload' && front?.kind !== 'download') return undefined; + return { kind: front.kind, name: front.name ?? '', done: 0, total: 0, preparing: true }; +} + /** Fold an `sftp-op-done` in: pop the front pending op (FIFO), record its refresh * target, clear the transfer display if it was a transfer, and surface any error. */ export function applyOpDone(session: SftpSession, ok: boolean, error?: string): SftpSession { From 5f598485ed10d98455dc4cac09759c4ca0f14f3d Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:09:23 +0400 Subject: [PATCH 16/44] fix(gui): show a pane loading while its listing waits on a transfer --- crates/omnyssh-gui/ui/e2e/sftp.spec.ts | 64 ++++++++++++++++--- .../ui/src/lib/screens/SftpPane.svelte | 11 +++- .../ui/src/lib/screens/SftpView.svelte | 6 +- 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts index 7536978f..4b7aa329 100644 --- a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts +++ b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts @@ -29,6 +29,18 @@ async function boot( // A pending transfer holds until the test fires its op-done, so the progress bar // is observable mid-flight (the core is sequential — one transfer at a time). const completions: Array<{ sessionId: number; tid: number; finish: () => void }> = []; + // Remote listings asked for meanwhile wait behind it, in the core's one queue. + const waiting: Array<{ sessionId: number; list: () => void }> = []; + function busy(sessionId: number): boolean { + return completions.some((c) => c.sessionId === sessionId); + } + function release(sessionId: number): void { + if (busy(sessionId)) return; + for (const w of waiting.filter((w) => w.sessionId === sessionId)) { + waiting.splice(waiting.indexOf(w), 1); + w.list(); + } + } type Entry = { name: string; @@ -100,8 +112,11 @@ async function boot( } // Fire the oldest still-pending transfer's op-done (deterministic completion). - (win as { __completeTransfer?: () => void }).__completeTransfer = () => - completions.shift()?.finish(); + (win as { __completeTransfer?: () => void }).__completeTransfer = () => { + const c = completions.shift(); + c?.finish(); + if (c) release(c.sessionId); + }; // A progress tick for the oldest pending transfer. function tick(done: number, total: number): void { const c = completions[0]; @@ -142,10 +157,13 @@ async function boot( } case 'sftp_list': { const { sessionId, path } = args as { sessionId: number; path: string }; - setTimeout( - () => fireEvent('sftp-dir-listed', { sessionId, path, entries: withParent(path, remote[path] ?? []) }), - 0 - ); + const list = () => + setTimeout( + () => fireEvent('sftp-dir-listed', { sessionId, path, entries: withParent(path, remote[path] ?? []) }), + 0 + ); + if (busy(sessionId)) waiting.push({ sessionId, list }); + else list(); return Promise.resolve(null); } case 'sftp_upload': { @@ -185,10 +203,10 @@ async function boot( const i = completions.findIndex((c) => c.sessionId === sessionId); if (i >= 0) { completions.splice(i, 1); - setTimeout( - () => fireEvent('sftp-op-done', { sessionId, ok: false, error: 'Transfer cancelled' }), - 0 - ); + setTimeout(() => { + fireEvent('sftp-op-done', { sessionId, ok: false, error: 'Transfer cancelled' }); + release(sessionId); + }, 0); } return Promise.resolve(null); } @@ -322,6 +340,32 @@ test('a transfer shows as preparing until its first tick, a folder of empty file await expect(remotePane.getByText('work')).toBeVisible(); }); +test('a remote folder opened during a transfer shows loading, then opens', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('notes.txt')).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + await localPane.getByRole('checkbox', { name: 'Mark notes.txt' }).click(); + await page.getByRole('button', { name: 'Upload' }).click(); + await expect(page.getByLabel('transfer progress')).toBeVisible(); + + // The listing waits behind the transfer. + await remotePane.getByTitle('var', { exact: true }).click(); + await expect(remotePane).toHaveAttribute('aria-busy', 'true'); + await expect(remotePane.getByText('Loading…')).toBeVisible(); + + // Once it is done the pane opens the folder asked for, not the one it was on. + await complete(page); + await expect(remotePane).toHaveAttribute('aria-busy', 'false'); + await expect(remotePane.getByTitle('/var', { exact: true })).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toHaveCount(0); + await page.waitForTimeout(100); + await expect(remotePane.getByTitle('/var', { exact: true })).toBeVisible(); +}); + test('the panes show modification times', async ({ page }) => { await boot(page); await page.getByTitle('files on web-1').click(); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte index f3c2e5a4..e5e98d0f 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpPane.svelte @@ -5,6 +5,7 @@ // batch transfer/delete. A press on a row may start a drag (the parent owns the drag // and marks the pane `dropActive` while it is the target); `data-pane` and // `data-dir-path` are how the parent finds the pane and folder under the pointer. + // A listing on its way dims the current one: a remote one can wait behind a transfer. // Semantic tokens only — no colour literals (§5.1). import type { Snippet } from 'svelte'; import { Icon } from '$lib/theme'; @@ -46,6 +47,7 @@
    -
    - {pane.path || '—'} +
    + {pane.path || '—'} + {#if pane.loading && pane.entries.length > 0} + Loading… + {/if}

@@ -84,7 +89,7 @@ Size Modified -
    +
      {#each pane.entries as entry, i (i)} {@const isParent = entry.name === '..'} {@const marked = pane.marked.has(entry.path)} diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index 116b3c6a..f7c74aab 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -195,7 +195,11 @@ const target = view.refresh; sftp.clearRefresh(id); if (target === 'local' || target === 'both') void refreshLocal(view.local.path); - if (target === 'remote' || target === 'both') refreshRemote(view.remote.path); + // A remote listing asked for meanwhile waited in the core's queue behind the + // transfer, so it shows the change; the old path again would take the pane back. + if ((target === 'remote' || target === 'both') && !view.remote.loading) { + refreshRemote(view.remote.path); + } }); function navigate(side: PaneSide, entry: FileEntryDto): void { From e7308ab6b36182d0807df4d85b84363ff42dd564 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:12:11 +0400 Subject: [PATCH 17/44] refactor(gui): drop markedTransferable, which filtered nothing --- .../omnyssh-gui/ui/src/lib/screens/SftpView.svelte | 12 +++++------- crates/omnyssh-gui/ui/src/lib/stores/sftp.ts | 7 +------ 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index f7c74aab..04c6b2e6 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -17,7 +17,6 @@ import { sftp, markedEntries, - markedTransferable, dragPayload, formatBytes, rootOf, @@ -82,9 +81,8 @@ const transfer = $derived(view ? transferState(view) : undefined); // Folders transfer whole, so the Upload/Download buttons take them like files. - const localMarkedItems = $derived(view ? markedTransferable(view.local) : []); + const localMarked = $derived(view ? markedEntries(view.local) : []); const remoteMarked = $derived(view ? markedEntries(view.remote) : []); - const remoteMarkedItems = $derived(view ? markedTransferable(view.remote) : []); const singleRemoteMark = $derived(remoteMarked.length === 1 ? remoteMarked[0] : undefined); function errMsg(err: unknown): string { @@ -556,8 +554,8 @@ type="button" class={toolBtn} title="Upload marked files and folders to the remote directory" - disabled={localMarkedItems.length === 0} - onclick={() => upload(localMarkedItems)} + disabled={localMarked.length === 0} + onclick={() => upload(localMarked)} > Upload @@ -594,8 +592,8 @@ type="button" class={toolBtn} title="Download marked files and folders to the local directory" - disabled={remoteMarkedItems.length === 0} - onclick={() => download(remoteMarkedItems)} + disabled={remoteMarked.length === 0} + onclick={() => download(remoteMarked)} > Download diff --git a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts index f8b7b9cd..3c0c4c2b 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/sftp.ts @@ -95,17 +95,12 @@ export function markedEntries(pane: Pane): FileEntryDto[] { return pane.entries.filter((e) => pane.marked.has(e.path)); } -/** The marked entries a transfer carries: files and folders (copied whole), never `..`. */ -export function markedTransferable(pane: Pane): FileEntryDto[] { - return markedEntries(pane).filter((e) => e.name !== '..'); -} - /** What dragging `entry` carries: every marked entry when `entry` is one of them, else * `entry` alone. Folders travel with their whole contents; `..` is not transferable, * so dragging it carries nothing. */ export function dragPayload(pane: Pane, entry: FileEntryDto): FileEntryDto[] { if (entry.name === '..') return []; - if (pane.marked.has(entry.path)) return markedTransferable(pane); + if (pane.marked.has(entry.path)) return markedEntries(pane); return [entry]; } From f8f03975ef58e11965b9f9f010ff74083707cb50 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Fri, 2 Oct 2026 00:12:26 +0400 Subject: [PATCH 18/44] fix(gui): keep a pane drag to the active tab and off stray clicks --- crates/omnyssh-gui/ui/e2e/sftp.spec.ts | 128 ++++++++++++++++-- .../ui/src/lib/screens/SftpView.svelte | 72 +++++++--- 2 files changed, 170 insertions(+), 30 deletions(-) diff --git a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts index 4b7aa329..170d5b89 100644 --- a/crates/omnyssh-gui/ui/e2e/sftp.spec.ts +++ b/crates/omnyssh-gui/ui/e2e/sftp.spec.ts @@ -378,15 +378,29 @@ test('the panes show modification times', async ({ page }) => { await expect(remotePane.getByTitle(/^Modified .*2024/)).toBeVisible(); }); -async function dragOnto(page: Page, from: Locator, to: Locator): Promise { - const a = await from.boundingBox(); - const b = await to.boundingBox(); - if (!a || !b) throw new Error('drag endpoints not laid out'); - await page.mouse.move(a.x + a.width / 2, a.y + a.height / 2); +async function centre(locator: Locator): Promise<{ x: number; y: number }> { + const box = await locator.boundingBox(); + if (!box) throw new Error('not laid out'); + return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; +} + +// Press on `from` and move past the drag threshold, holding the button. +async function startDrag(page: Page, from: Locator): Promise { + const a = await centre(from); + await page.mouse.move(a.x, a.y); await page.mouse.down(); - // Several steps: the drag only starts past a small threshold, then tracks the target. - await page.mouse.move(a.x + a.width / 2 + 20, a.y + a.height / 2, { steps: 4 }); - await page.mouse.move(b.x + b.width / 2, b.y + b.height / 2, { steps: 8 }); + await page.mouse.move(a.x + 20, a.y, { steps: 4 }); +} + +async function dragOnto( + page: Page, + from: Locator, + to: Locator | { x: number; y: number } +): Promise { + await startDrag(page, from); + // Several steps, so the drag tracks the target. + const b = 'x' in to ? to : await centre(to); + await page.mouse.move(b.x, b.y, { steps: 8 }); await page.mouse.up(); } @@ -433,6 +447,73 @@ test('drag and drop: dropping back on the same pane transfers nothing', async ({ expect(uploads).toEqual([]); }); +test('drag and drop: Escape drops the drag, and the release after it clicks nothing', async ({ + page +}) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const remotePane = page.getByRole('region', { name: 'web-1' }); + const folder = remotePane.getByTitle('var', { exact: true }); + await expect(folder).toBeVisible(); + + await startDrag(page, folder); + const named = page.getByText('var', { exact: true }).filter({ visible: true }); + await expect(named).toHaveCount(2); // the row and the ghost + await page.keyboard.press('Escape'); + await expect(named).toHaveCount(1); + + // Back on the row it started from: no click, so the pane stays where it was. + const back = await centre(folder); + await page.mouse.move(back.x, back.y, { steps: 4 }); + await page.mouse.up(); + await page.waitForTimeout(100); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + const downloads = await page.evaluate(() => (window as unknown as { __downloads: string[] }).__downloads); + expect(downloads).toEqual([]); +}); + +test('drag and drop: `..` dragged away and back does not go up, like any row', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + await localPane.getByTitle('work', { exact: true }).click(); + await expect(localPane.getByTitle('/home/user/work', { exact: true })).toBeVisible(); + + const parent = localPane.getByTitle('..', { exact: true }); + const at = await centre(parent); + await page.mouse.move(at.x, at.y); + await page.mouse.down(); + await page.mouse.move(at.x, at.y + 200, { steps: 6 }); + await page.mouse.move(at.x, at.y, { steps: 6 }); + await page.mouse.up(); + await page.waitForTimeout(100); + await expect(localPane.getByTitle('/home/user/work', { exact: true })).toBeVisible(); +}); + +test('drag and drop: a drop on the transfer strip goes to the pane above it', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + const localPane = page.getByRole('region', { name: 'Local' }); + const remotePane = page.getByRole('region', { name: 'web-1' }); + await expect(localPane.getByText('notes.txt')).toBeVisible(); + await expect(remotePane.getByText('config.yml')).toBeVisible(); + + await localPane.getByRole('checkbox', { name: 'Mark notes.txt' }).click(); + await page.getByRole('button', { name: 'Upload' }).click(); + const strip = page.getByLabel('transfer progress'); + await expect(strip).toBeVisible(); + + // Released on the strip's local half: a download into the local pane's directory, + // queued behind the upload. + const x = (await centre(localPane)).x; + const y = (await centre(strip)).y; + await dragOnto(page, remotePane.getByTitle('config.yml', { exact: true }), { x, y }); + await complete(page); + await expect + .poll(() => page.evaluate(() => (window as unknown as { __downloads: string[] }).__downloads)) + .toEqual(['/home/user/config.yml']); +}); + test('drag and drop: a remote folder dropped on the local pane downloads it whole', async ({ page }) => { await boot(page); await page.getByTitle('files on web-1').click(); @@ -501,6 +582,37 @@ test('an inactive tab’s modal never overlays another entity (§2 exactly-one-a await expect(page.getByRole('dialog', { name: 'New folder' })).toHaveCount(0); }); +test('a drag lets go when its tab is hidden (§2 exactly-one-active)', async ({ page }) => { + await boot(page); + await page.getByTitle('files on web-1').click(); + await expect(page.getByRole('region', { name: 'web-1' }).getByText('config.yml')).toBeVisible(); + await page.getByRole('button', { name: 'Dashboard', exact: true }).click(); + await page.getByTitle('files on db-1').click(); + const dbRemote = page.getByRole('region', { name: 'db-1' }); + await expect(dbRemote.getByText('config.yml')).toBeVisible(); + await page.getByRole('button', { name: 'web-1 · sftp', exact: true }).click(); + + // A drag in web-1, and db-1 brought up with the keyboard while the button is held. + const localPane = page.getByRole('region', { name: 'Local' }); + await startDrag(page, localPane.getByTitle('notes.txt', { exact: true })); + const named = page.getByText('notes.txt', { exact: true }).filter({ visible: true }); + await expect(named).toHaveCount(2); // the row and the ghost + await page.keyboard.press('Control+k'); + await page.getByRole('dialog', { name: 'Command palette' }).getByRole('textbox').fill('db-1'); + await page.keyboard.press('Enter'); + await expect(dbRemote).toBeVisible(); + + // No ghost over db-1, and a release on its remote pane sends nothing. + await expect(named).toHaveCount(1); + const over = await centre(dbRemote.getByText('config.yml')); + await page.mouse.move(over.x, over.y, { steps: 4 }); + await page.mouse.up(); + await page.waitForTimeout(100); + const uploads = await page.evaluate(() => (window as unknown as { __uploads: string[] }).__uploads); + expect(uploads).toEqual([]); + await expect(page.getByLabel('transfer progress')).toHaveCount(0); +}); + test('action-first: the SFTP spawner opens the host picker, then a live session', async ({ page }) => { diff --git a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte index 04c6b2e6..a8db2c76 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/SftpView.svelte @@ -294,6 +294,7 @@ row?: string; } + let root = $state(); let press: { side: PaneSide; entry: FileEntryDto; x: number; y: number } | null = null; let drag = $state<{ from: PaneSide; @@ -302,28 +303,59 @@ y: number; target: DropTarget | null; } | null>(null); - // A drag that ends on the row it started on would otherwise click it. + // The press went past the threshold, carrying something or not (`..`): the release + // must not click the row it ends on, even after Escape. + let moved = false; let swallowClick = false; // A drag from the OS hovering this view, for the remote pane's highlight. let osDrop = $state(null); - /** The pane (and folder row, if any) under a viewport point. */ + function endGesture(): void { + press = null; + drag = null; + moved = false; + } + + // A hidden view lets go of what it held (§2): no ghost over the entity now shown, and + // no transfer from a release there. + $effect(() => { + if (!active) { + endGesture(); + osDrop = null; + } + }); + + /** This view's pane (and folder row, if any) under a viewport point. Below the panes, + * on the transfer strip, a point goes to the pane above it. */ function dropTargetAt(x: number, y: number): DropTarget | null { - if (!view) return null; + if (!view || !root) return null; const el = document.elementFromPoint(x, y); - const paneEl = el?.closest('[data-pane]'); + if (!el || !root.contains(el)) return null; + const paneEl = + el.closest('[data-pane]') ?? + [...root.querySelectorAll('[data-pane]')].find((pane) => { + const r = pane.getBoundingClientRect(); + return x >= r.left && x < r.right; + }); const side = paneEl?.dataset.pane; if (side !== 'local' && side !== 'remote') return null; - const row = el?.closest('[data-dir-path]')?.dataset.dirPath; + const row = el.closest('[data-dir-path]')?.dataset.dirPath; return { side, dir: row ?? view[side].path, row }; } function startPress(side: PaneSide, entry: FileEntryDto, e: PointerEvent): void { - if (e.button !== 0 || !view) return; + if (e.button !== 0 || !view || !active) return; press = { side, entry, x: e.clientX, y: e.clientY }; + moved = false; } function onPointerMove(e: PointerEvent): void { + if (!press && !drag) return; + // The button came up where no pointerup reached this window. + if ((e.buttons & 1) === 0) { + endGesture(); + return; + } if (drag) { const target = dropTargetAt(e.clientX, e.clientY); drag = { @@ -336,6 +368,7 @@ } if (!press || !view) return; if (Math.hypot(e.clientX - press.x, e.clientY - press.y) < DRAG_THRESHOLD) return; + moved = true; const files = dragPayload(view[press.side], press.entry); const from = press.side; press = null; @@ -344,22 +377,19 @@ } function onPointerUp(): void { - press = null; const done = drag; - if (!done) return; - drag = null; - swallowClick = true; - setTimeout(() => (swallowClick = false), 0); - if (!done.target) return; + if (moved) { + swallowClick = true; + setTimeout(() => (swallowClick = false), 0); + } + endGesture(); + if (!active || !done?.target) return; if (done.from === 'local') upload(done.files, done.target.dir); else download(done.files, done.target.dir); } function onKeyDown(e: KeyboardEvent): void { - if (e.key === 'Escape' && drag) { - drag = null; - press = null; - } + if (e.key === 'Escape' && drag) drag = null; } // Drops from the OS (Explorer, Finder): Tauri reports physical pixels and absolute @@ -484,18 +514,15 @@ { - press = null; - drag = null; - }} + onpointercancel={endGesture} onkeydown={onKeyDown} /> -{#if drag} +{#if active && drag}