From f566162c29c4863c005b4ff9d4bdfe94a5eae809 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Thu, 10 Sep 2026 19:35:03 +0200 Subject: [PATCH] review pass: mount safety, abort/quit robustness, UI fixes Engine / safety: - verify mount points against /proc/self/mountinfo instead of path existence; refuse to sync into a leftover (unmounted) directory that would otherwise receive the library on the root filesystem - resolve udisks block devices by label when the path is not mounted, and never let findmnt fall back to the containing root filesystem - abort now SIGTERMs the whole child process group (sh -c/podkit and rsync children included), escalating to SIGKILL after 2s - quitting while a sync runs aborts and waits for the engine instead of orphaning the child - atomically mark the engine busy before spawning to prevent double syncs; persist state before publishing completion - don't follow symlinked directories while scanning (recursion loops) - prune empty directories recursively when removing stale files UI: - help overlay was clipped (hardcoded height): size it from content - speed/ETA were pushed onto a line the progress panel clipped; show them right-aligned on the bytes row - leaving log follow with u/d now scrolls from the bottom instead of jumping to the top of the log - reload config with r (label updated), refresh mounts on completion so last-sync ages are current - fix width underflows on narrow terminals; terminal-aware sidebar scrolling Tests: mountinfo unescape, process-group abort, and a TestBackend render smoke test covering all tabs, small terminals and the help overlay. --- README.md | 13 ++- src/app.rs | 87 ++++++++++++++----- src/config.rs | 7 +- src/disk.rs | 56 +++++++++++- src/engine.rs | 178 +++++++++++++++++++++++++++++++++------ src/udisks.rs | 30 +++++-- src/ui/components.rs | 4 +- src/ui/mod.rs | 162 ++++++++++++++++++++++++++--------- src/ui/tabs/dashboard.rs | 2 +- src/ui/tabs/logs.rs | 1 + src/ui/tabs/sync.rs | 68 +++++++-------- 11 files changed, 470 insertions(+), 138 deletions(-) diff --git a/README.md b/README.md index cb9d517..9792406 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,11 @@ living dashboard instead of a batch CLI. - **Live sync engine** — streams `rclone` and `rsync` output, parses progress (byte-weighted via `--info=progress2`), NFC-aware diffing for macOS→FAT32/Android devices, optional `fatsort` finalize for Mlove devices. -- **Abort-safe** — `a` kills the running child process cleanly. +- **Abort-safe** — `a` kills the running child process group cleanly; quitting + while a sync runs aborts it first instead of leaving it orphaned. +- **Mount-aware** — a path that exists but is not a real mount point is flagged + and refused as a sync target, so a leftover mount directory can't silently + receive a sync on the root filesystem. - Reads the existing `~/.config/dap-sync/config.toml` (compatible with `dap-sync`), persists last-sync state to `~/.local/share/dap-sync/sync-state.toml`. @@ -41,7 +45,7 @@ cargo build --release | `a` | abort running sync | | `m` | mount / unmount device (udisks2, no sudo) | | `Tab` / `1-3`| switch tab | -| `r` | refresh device mounts | +| `r` | reload config + refresh mounts | | `f` | toggle log follow | | `u` / `d` | scroll logs | | `x` | clear logs | @@ -91,6 +95,11 @@ Devices are mounted/unmounted without sudo via **udisks2**: mounts the target first (resolved by mount-point label via `lsblk`). Only the mlove `fatsort` step needs root (raw block device). +Mount status and sync targets are validated against the kernel mount table +(`/proc/self/mountinfo`), not just directory existence: if the configured +`mount_point` exists but is not actually mounted, `dap-tui` refuses to sync to +it rather than writing the library onto the root filesystem. + ## Development ```bash diff --git a/src/app.rs b/src/app.rs index 98f8c69..8a32500 100644 --- a/src/app.rs +++ b/src/app.rs @@ -48,6 +48,9 @@ pub struct App { pub logs: Vec, pub log_follow: bool, pub log_scroll: usize, + /// Height of the log viewport as last rendered (used to scroll from the + /// bottom when leaving follow mode). + pub log_view: std::cell::Cell, pub frame: u64, pub should_quit: bool, @@ -68,6 +71,7 @@ impl App { logs: Vec::new(), log_follow: true, log_scroll: 0, + log_view: std::cell::Cell::new(0), frame: 0, should_quit: false, show_help: false, @@ -88,16 +92,6 @@ impl App { self.engine.running() } - fn refresh_mounts(&mut self) { - for d in self.config.devices.iter_mut() { - d.mounted = d - .mount_point - .as_ref() - .map(|p| p.exists()) - .unwrap_or(false); - } - } - fn drain_engine(&mut self) { while let Ok(ev) = self.engine.events.try_recv() { match ev { @@ -116,9 +110,17 @@ impl App { } } EngineEvent::Changed => {} - EngineEvent::Finished { ok, summary } => { + EngineEvent::Finished { device, ok, summary } => { + // Refresh in-memory last-sync state so the dashboard shows + // the run that just finished instead of the stale time + // loaded at startup (the engine already persisted it). + if let Some(d) = self.config.devices.iter_mut().find(|d| d.name == device) { + d.last_sync = Some(chrono::Local::now()); + d.last_status = if ok { "ok" } else { "error" }.to_string(); + } + self.config.refresh_mounts(); let level = if ok { LogLevel::Success } else { LogLevel::Error }; - self.status_msg = Some((summary.clone(), level)); + self.status_msg = Some((summary, level)); } } } @@ -170,7 +172,7 @@ impl App { self.status_msg = Some((format!("{action} failed — {e}"), LogLevel::Error)); } } - self.refresh_mounts(); + self.config.refresh_mounts(); } pub fn on_key(&mut self, key: KeyEvent) { @@ -203,10 +205,7 @@ impl App { KeyCode::Char('s') | KeyCode::Enter => self.start_sync(), KeyCode::Char('a') => self.abort_sync(), KeyCode::Char('m') => self.toggle_mount(), - KeyCode::Char('r') => { - self.refresh_mounts(); - self.status_msg = Some(("devices refreshed".to_string(), LogLevel::Info)); - } + KeyCode::Char('r') => self.reload_config(), KeyCode::Char('f') => { self.log_follow = !self.log_follow; self.log_scroll = 0; @@ -236,25 +235,60 @@ impl App { } fn ensure_selection_visible(&mut self) { - const VISIBLE: usize = 12; + // Sidebar rows available: terminal height − title(2) − rule(1) − + // status(1) − keys(1) − sidebar head/foot(2), in 2-line device rows. + let visible = ratatui::crossterm::terminal::size() + .map(|(_, h)| ((h.saturating_sub(7) / 2).max(1)) as usize) + .unwrap_or(12); if self.selected < self.sidebar_scroll { self.sidebar_scroll = self.selected; } - if self.selected >= self.sidebar_scroll + VISIBLE { - self.sidebar_scroll = self.selected + 1 - VISIBLE; + if self.selected >= self.sidebar_scroll + visible { + self.sidebar_scroll = self.selected + 1 - visible; } } fn scroll_logs(&mut self, delta: i32) { - self.log_follow = false; + if self.log_follow { + // Already pinned to the bottom: scrolling further down is a no-op, + // while scrolling up must start from the bottom of the buffer + // instead of jumping back to the top of the log. + if delta > 0 { + return; + } + let page = self.log_view.get().max(1); + self.log_scroll = self.logs.len().saturating_sub(page); + self.log_follow = false; + } let max = self.logs.len().saturating_sub(1) as i32; self.log_scroll = ((self.log_scroll as i32 + delta).clamp(0, max)) as usize; } + fn reload_config(&mut self) { + let path = self.config.config_file.display().to_string(); + match crate::config::load(Some(path)) { + Ok(cfg) => { + self.config = cfg; + if self.selected >= self.config.devices.len() { + self.selected = self.config.devices.len().saturating_sub(1); + } + self.sidebar_scroll = 0; + self.ensure_selection_visible(); + self.status_msg = Some(( + "config reloaded · devices refreshed".to_string(), + LogLevel::Info, + )); + } + Err(e) => { + self.status_msg = Some((format!("config reload failed — {e}"), LogLevel::Error)); + } + } + } + fn tick(&mut self) { self.frame += 1; if self.frame % 30 == 0 { - self.refresh_mounts(); + self.config.refresh_mounts(); } if self.is_syncing() { let aborted = self.engine.abort.load(Ordering::Relaxed); @@ -282,6 +316,15 @@ impl App { } Ok(()) })(); + // Never leave a sync child process orphaned when the user quits: + // request an abort and give the engine a moment to terminate it. + if self.engine.running() { + self.engine.abort_sync(); + let deadline = std::time::Instant::now() + Duration::from_secs(3); + while self.engine.running() && std::time::Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(50)); + } + } ratatui::restore(); res } diff --git a/src/config.rs b/src/config.rs index cc956e8..a6886c5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -130,12 +130,13 @@ pub struct Config { } impl Config { - fn refresh_mounts(&mut self) { + /// Refresh `Device::mounted` from the kernel mount table (no subprocess). + pub fn refresh_mounts(&mut self) { for d in self.devices.iter_mut() { d.mounted = d .mount_point - .as_ref() - .map(|p| p.exists()) + .as_deref() + .map(crate::disk::is_mounted) .unwrap_or(false); } } diff --git a/src/disk.rs b/src/disk.rs index 56dffe4..764c587 100644 --- a/src/disk.rs +++ b/src/disk.rs @@ -1,5 +1,52 @@ use std::path::Path; +/// Whether `path` is currently a mount point. +/// +/// Scanning `/proc/self/mountinfo` is used instead of `path.exists()` so a +/// leftover mount-point directory (e.g. `/media/sebastian/IPOD` after an +/// unclean eject) is not mistaken for a mounted device. Writing a device sync +/// into an unmounted directory would silently fill the root filesystem. +pub fn is_mounted(path: &Path) -> bool { + let Some(target) = path.to_str() else { + return false; + }; + let target = target.trim_end_matches('/'); + + #[cfg(target_os = "linux")] + if let Ok(text) = std::fs::read_to_string("/proc/self/mountinfo") { + return text.lines().any(|line| { + let Some(field) = line.split(' ').nth(4) else { + return false; + }; + unescape_octal(field).trim_end_matches('/') == target + }); + } + + // Fallback for platforms without /proc: best effort only. + path.exists() +} + +/// Undo the octal escaping (`\040` etc.) used by /proc mount tables. +fn unescape_octal(s: &str) -> String { + let bytes = s.as_bytes(); + let mut out = String::with_capacity(s.len()); + let mut i = 0; + while i < bytes.len() { + let octal = bytes[i] == b'\\' + && i + 3 < bytes.len() + && bytes[i + 1..=i + 3].iter().all(|b| (b'0'..=b'7').contains(b)); + if octal { + let v = (bytes[i + 1] - b'0') * 64 + (bytes[i + 2] - b'0') * 8 + (bytes[i + 3] - b'0'); + out.push(v as char); + i += 4; + } else { + out.push(bytes[i] as char); + i += 1; + } + } + out +} + /// Total and available bytes on a mounted filesystem (statvfs). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct DiskStats { @@ -37,7 +84,14 @@ pub fn fmt_bytes(n: u64) -> String { #[cfg(test)] mod tests { - use super::fmt_bytes; + use super::{fmt_bytes, unescape_octal}; + + #[test] + fn unescapes_mountinfo_paths() { + assert_eq!(unescape_octal("/media/sebastian/IPOD"), "/media/sebastian/IPOD"); + assert_eq!(unescape_octal("/media/My\\040Disk"), "/media/My Disk"); + assert_eq!(unescape_octal("/media/back\\134slash"), "/media/back\\slash"); + } #[test] fn formats_bytes() { diff --git a/src/engine.rs b/src/engine.rs index 7861eac..5a58e55 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -83,6 +83,8 @@ pub enum EngineEvent { Log(LogEntry), Changed, Finished { + /// Name of the device the finished sync targeted. + device: String, ok: bool, summary: String, }, @@ -180,8 +182,19 @@ impl Engine { } pub fn start_sync(&self, device: Device, config: Arc, skip_mirror: bool) { - if self.running() { - return; + // Mark the engine busy *before* spawning. Otherwise a double keypress + // could pass the `running()` check twice and start two concurrent + // syncs (the worker used to be the one setting `running`). + match self.state.lock() { + Ok(mut s) => { + if s.running { + return; + } + s.running = true; + s.device = Some(device.name.clone()); + s.summary = "Starting sync…".to_string(); + } + Err(_) => return, } self.abort.store(false, Ordering::Relaxed); let ctx = Arc::new(WorkerCtx { @@ -243,6 +256,14 @@ fn run_sync(config: Arc, device: Device, skip_mirror: bool, ctx: Arc, device: Device, skip_mirror: bool, ctx: Arc Result<(), String> { - let steps = build_steps(device); - if !skip_mirror { ctx.set_step(0, StepStatus::Active, "refreshing local library…"); run_mirror(config, device, ctx)?; @@ -312,7 +326,6 @@ fn run_pipeline( } } - let _ = steps; Ok(()) } @@ -445,6 +458,11 @@ fn scan_music_dir(base: &Path) -> HashMap { } let path = entry.path(); if path.is_dir() { + // Never follow symlinked directories: a device can contain + // self-referential links and we would recurse forever. + if entry.file_type().map(|t| t.is_symlink()).unwrap_or(false) { + continue; + } stack.push(path); continue; } @@ -479,21 +497,26 @@ fn run_rsync_device(config: &Config, device: &Device, ctx: &WorkerCtx) -> Result .as_ref() .ok_or_else(|| "mount_point is not set for this device".to_string())?; - if !mount.exists() { - ctx.log(LogLevel::Info, format!("device not mounted at {} — trying udisksctl…", mount.display())); + if !crate::disk::is_mounted(mount) { + ctx.log( + LogLevel::Info, + format!("device not mounted at {} — trying udisksctl…", mount.display()), + ); match crate::udisks::mount_device(mount) { Ok(msg) => ctx.log(LogLevel::Info, format!("auto-mounted: {msg}")), Err(e) => { return Err(format!( - "device not found at {} — connect & mount it (m) and retry. {e}", + "device not mounted at {} — connect & mount it (m) and retry. {e}", mount.display() )); } } } - if !mount.exists() { + // A leftover (empty) mount-point directory must not be mistaken for a + // mounted device — rsync would otherwise happily fill the root filesystem. + if !crate::disk::is_mounted(mount) { return Err(format!( - "device not found at {} — connect & mount it (m) and retry", + "{} exists but is not a mount point — refusing to sync. Connect the device and press 'm'.", mount.display() )); } @@ -626,8 +649,13 @@ fn remove_stale(dest: &Path, to_delete: &[String]) { if let Some(p) = actual.get(key) { if p.exists() { let _ = fs::remove_file(p); - if let Some(parent) = p.parent() { - let _ = fs::remove_dir(parent); // remove if now empty + // Prune directories that are now empty, up to (not including) dest. + let mut parent = p.parent(); + while let Some(d) = parent { + if d == dest || fs::remove_dir(d).is_err() { + break; + } + parent = d.parent(); } } } @@ -743,6 +771,33 @@ fn pump_lines( } } +/// Put the child in its own process group so a whole pipeline (e.g. +/// `sh -c podkit …`, or rsync's own children) can be signalled on abort. +#[cfg(unix)] +fn detach_process_group(command: &mut Command) { + use std::os::unix::process::CommandExt; + unsafe { + command.pre_exec(|| { + libc::setpgid(0, 0); + Ok(()) + }); + } +} + +/// Signal the process group of `child` (SIGTERM, or SIGKILL when `force`). +fn terminate_process_group(child: &mut std::process::Child, force: bool) { + #[cfg(unix)] + unsafe { + let sig = if force { libc::SIGKILL } else { libc::SIGTERM }; + libc::kill(-(child.id() as i32), sig); + } + #[cfg(not(unix))] + { + let _ = force; + let _ = child.kill(); + } +} + /// Run a command, streaming its output lines to `on_line`. Returns Err on /// failure or abort. Lines returned `true` from `on_line` are treated as /// consumed (progress); unconsumed lines are logged. @@ -759,11 +814,16 @@ fn stream( where F: FnMut(&str) -> bool, { - let mut child = Command::new(cmd) + let mut command = Command::new(cmd); + command .args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) - .stderr(Stdio::piped()) + .stderr(Stdio::piped()); + #[cfg(unix)] + detach_process_group(&mut command); + + let mut child = command .spawn() .map_err(|e| format!("could not start {cmd}: {e}"))?; @@ -775,11 +835,19 @@ where let out_thread = thread::spawn(move || pump_lines(stdout, false, out_tx)); let err_thread = thread::spawn(move || pump_lines(stderr, true, tx)); + // On abort, ask the whole process group to terminate; escalate to SIGKILL + // if the pipes have not closed within a grace period. + let mut terminating: Option = None; loop { - if ctx.abort.load(Ordering::Relaxed) { - let _ = child.kill(); - let _ = child.wait(); - return Err("aborted".to_string()); + if ctx.abort.load(Ordering::Relaxed) && terminating.is_none() { + terminate_process_group(&mut child, false); + terminating = Some(std::time::Instant::now()); + } + if let Some(started) = terminating { + if started.elapsed() > std::time::Duration::from_secs(2) { + terminate_process_group(&mut child, true); + break; + } } match rx.recv_timeout(std::time::Duration::from_millis(200)) { Ok((is_stderr, line)) => { @@ -853,4 +921,66 @@ mod tests { assert!(seen[0].contains("first stats line")); assert!(seen[1].contains("second stats line")); } + + #[cfg(unix)] + fn process_is_alive(pid: i32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + // `pid (comm) state …` — comm may itself contain spaces/parens, so + // split at the *last* `) ` and read the state character. + Ok(stat) => stat + .rsplit(") ") + .next() + .map(|rest| !rest.starts_with('Z')) + .unwrap_or(false), + Err(_) => false, + } + } + + #[test] + #[cfg(unix)] + fn abort_terminates_whole_process_group() { + // `sh -c` spawns a long-running grandchild; aborting must terminate + // the whole group, not just the direct shell child (which used to be + // left orphaned and running when the user pressed `a` or quit). + let (tx, _rx) = mpsc::channel::(); + let ctx = Arc::new(WorkerCtx { + state: Arc::new(Mutex::new(SyncState::default())), + tx, + abort: Arc::new(AtomicBool::new(false)), + }); + let grandchild: Arc>> = Arc::new(Mutex::new(None)); + let capture = grandchild.clone(); + let args: Vec = vec![ + "-c".to_string(), + "sleep 30 & echo CHILD:$!; wait".to_string(), + ]; + + let abort = ctx.abort.clone(); + thread::spawn(move || { + thread::sleep(std::time::Duration::from_millis(400)); + abort.store(true, Ordering::Relaxed); + }); + + let start = std::time::Instant::now(); + let result = stream(&ctx, "sh", &args, move |line| { + if let Some(pid) = line.strip_prefix("CHILD:") { + *capture.lock().unwrap() = pid.trim().parse().ok(); + } + true + }); + assert!(result.is_err(), "aborted stream must return an error"); + assert!( + start.elapsed() < std::time::Duration::from_secs(5), + "abort must not hang" + ); + + let pid = grandchild.lock().unwrap().expect("grandchild pid captured"); + for _ in 0..20 { + if !process_is_alive(pid) { + return; + } + thread::sleep(std::time::Duration::from_millis(50)); + } + panic!("grandchild process {pid} survived the abort"); + } } diff --git a/src/udisks.rs b/src/udisks.rs index 8c128b3..026f468 100644 --- a/src/udisks.rs +++ b/src/udisks.rs @@ -19,7 +19,10 @@ fn run(cmd: &str, args: &[&str]) -> (bool, String) { } fn resolve_block_device(path: &Path) -> Option { - if path.is_dir() { + // Only trust findmnt when the path really is a mount point; otherwise + // findmnt happily reports the containing filesystem (e.g. /), which would + // make us "mount" the root device. + if crate::disk::is_mounted(path) { let (ok, out) = run("findmnt", &["-n", "-o", "SOURCE", "-T", path.to_str()?]); if ok { let src = out.trim(); @@ -29,22 +32,33 @@ fn resolve_block_device(path: &Path) -> Option { } } let wanted = path.file_name()?.to_string_lossy().to_lowercase(); - let (ok, out) = run("lsblk", &["-o", "PATH,LABEL", "-n", "-l"]); + let (ok, out) = run("lsblk", &["-o", "PATH,LABEL", "-n", "-l", "-P"]); if !ok { return None; } for line in out.lines() { - let mut parts = line.split_whitespace(); - let Some(dev) = parts.next() else { continue }; - if let Some(label) = parts.next() { - if !label.is_empty() && label.to_lowercase() == wanted { - return Some(dev.to_string()); - } + let Some(dev) = field(line, "PATH") else { + continue; + }; + let Some(label) = field(line, "LABEL") else { + continue; + }; + if !label.is_empty() && label.to_lowercase() == wanted { + return Some(dev); } } None } +/// Extract a `KEY="value"` field from `lsblk -P` output. +fn field(line: &str, key: &str) -> Option { + let needle = format!("{key}=\""); + let start = line.find(&needle)? + needle.len(); + let rest = &line[start..]; + let end = rest.find('"')?; + Some(rest[..end].to_string()) +} + pub fn mount_device(path: &Path) -> Result { let Some(dev) = resolve_block_device(path) else { return Err(format!("no block device found for {}", path.display())); diff --git a/src/ui/components.rs b/src/ui/components.rs index c57edc0..3d89627 100644 --- a/src/ui/components.rs +++ b/src/ui/components.rs @@ -205,7 +205,7 @@ fn draw_device_list(f: &mut Frame, area: Rect, app: &App, syncing: bool) { // selection marker + name let marker = if selected { "▸ " } else { " " }; - let name = truncate(&device.name, row.width.max(1) as usize - 8); + let name = truncate(&device.name, (row.width as usize).saturating_sub(8)); spans.push(Span::styled( format!("{marker}{name}"), Style::default().add_modifier(if selected { Modifier::BOLD } else { Modifier::default() }), @@ -358,7 +358,7 @@ pub fn draw_keys(f: &mut Frame, area: Rect, _app: &App) { ("a", "abort"), ("m", "mount"), ("Tab/1-3", "tab"), - ("r", "refresh"), + ("r", "reload"), ("f", "follow"), ("?", "help"), ("q", "quit"), diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 41ebd46..78585ec 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -5,10 +5,11 @@ pub mod theme; use crate::app::App; use ratatui::Frame; use ratatui::layout::{Constraint, Layout, Rect}; -use ratatui::style::{Style, Stylize}; -use ratatui::widgets::{Block, Borders, Clear}; +use ratatui::style::{Modifier, Style}; +use ratatui::text::{Line, Span}; +use ratatui::widgets::{Block, Borders, BorderType, Clear, Paragraph}; -use theme::{BG, DIM}; +use theme::BG; pub fn truncate(s: &str, max: usize) -> String { if s.chars().count() <= max { @@ -60,47 +61,126 @@ fn draw_main(f: &mut Frame, area: Rect, app: &App) { } fn draw_help(f: &mut Frame) { + let entries: &[(&str, &str)] = &[ + ("↑↓ / j k", "move selection"), + ("s / Enter", "sync selected device"), + ("a", "abort running sync"), + ("m", "mount / unmount device (udisks)"), + ("Tab / 1-3", "switch tab"), + ("r", "reload config · refresh mounts"), + ("f", "toggle log follow"), + ("u / d", "scroll logs"), + ("x", "clear logs"), + ("? / h", "this help"), + ("q / Esc", "quit"), + ]; + let area = f.area(); - let box_w = 58; - let box_h = 16; - let x = (area.width.saturating_sub(box_w)) / 2; - let y = (area.height.saturating_sub(box_h)) / 2; - let rect = Rect::new( - x.max(0), - y.max(0), - box_w.min(area.width), - box_h.min(area.height), - ); + let box_w: u16 = 52.min(area.width); + // blank + entries + blank + footer hint + two borders + let box_h: u16 = (entries.len() as u16 + 5).min(area.height); + let x = area.width.saturating_sub(box_w) / 2; + let y = area.height.saturating_sub(box_h) / 2; + let rect = Rect::new(x, y, box_w, box_h); f.render_widget(Clear, rect); + let block = Block::default() .borders(Borders::ALL) - .border_style(Style::default().fg(crate::ui::theme::ACCENT)) - .style(Style::default().bg(crate::ui::theme::PANEL_BG)) - .title(" KEYBINDINGS ".cyan().bold()); - let lines = [ - " ↑↓ / j k move selection", - " s / Enter sync selected device", - " a abort running sync", - " m mount / unmount selected device (udisks)", - " Tab / 1-3 switch tab", - " r refresh devices", - " f toggle log follow", - " u / d scroll logs", - " x clear logs", - " ? / h this help", - " q / Esc quit", - ]; - let mut spans = Vec::new(); - for (i, line) in lines.iter().enumerate() { - if i > 0 { - spans.push(ratatui::text::Line::raw("")); - } - let styled = if line.contains('/') { - ratatui::text::Span::styled(*line, ratatui::style::Style::default().fg(crate::ui::theme::FG)) - } else { - ratatui::text::Span::styled(*line, ratatui::style::Style::default().fg(DIM)) - }; - spans.push(ratatui::text::Line::from(styled)); + .border_type(BorderType::Rounded) + .border_style(Style::default().fg(theme::ACCENT)) + .style(Style::default().bg(theme::PANEL_BG)) + .title(Span::styled( + " KEYBINDINGS ", + Style::default().fg(theme::CYAN).add_modifier(Modifier::BOLD), + )); + + let mut lines: Vec = vec![Line::raw("")]; + for (key, desc) in entries { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + format!("{key:<11}"), + Style::default().fg(theme::CYAN).add_modifier(Modifier::BOLD), + ), + Span::styled(*desc, Style::default().fg(theme::FG)), + ])); + } + lines.push(Line::raw("")); + lines.push(Line::from(Span::styled( + " press Esc / ? / q to close", + Style::default().fg(theme::DIM), + ))); + + f.render_widget( + Paragraph::new(lines) + .block(block) + .style(Style::default().bg(theme::PANEL_BG)), + rect, + ); +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::app::App; + use crate::config::{Config, Device, Firmware}; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + use std::path::PathBuf; + + fn test_app() -> App { + let config = Config { + config_file: PathBuf::from("/tmp/dap-tui-test/config.toml"), + local_music_dir: PathBuf::from("/tmp/dap-tui-test/music"), + local_hoerspiele_dir: PathBuf::from("/tmp/dap-tui-test/hoerspiele"), + output_dir: PathBuf::from("/tmp/dap-tui-test/state"), + storagebox_source: "storagebox:test".into(), + storagebox_hoerspiele_source: "storagebox:test-hoerspiele".into(), + storagebox_excludes: vec!["/Music/**".into()], + devices: vec![Device { + name: "Test Player".into(), + firmware: Firmware::Rockbox, + content_type: "music".into(), + sync_command: "true".into(), + mount_point: Some(PathBuf::from("/tmp/dap-tui-test/not-mounted")), + music_folder: "Music".into(), + preserve_fat_order: false, + mounted: false, + last_sync: None, + last_status: String::new(), + }], + }; + App::new(config, false).unwrap() + } + + fn render(app: &App, width: u16, height: u16) -> String { + let mut terminal = Terminal::new(TestBackend::new(width, height)).unwrap(); + terminal.draw(|f| draw(f, app)).unwrap(); + terminal + .backend() + .buffer() + .content() + .iter() + .map(|c| c.symbol()) + .collect() + } + + #[test] + fn renders_all_tabs_and_help_without_panicking() { + // Small terminals used to underflow widths; the help overlay used to + // clip its last entries because the hardcoded height was too small. + for (w, h) in [(40u16, 10u16), (80, 24), (160, 48)] { + let mut app = test_app(); + for tab in crate::app::Tab::ALL { + app.tab = tab; + let _ = render(&app, w, h); + } + app.show_help = true; + let out = render(&app, w, h); + if h >= 24 { + assert!(out.contains("KEYBINDINGS"), "help title missing at {w}x{h}"); + assert!(out.contains("quit"), "last help entry clipped at {w}x{h}"); + } + } } - f.render_widget(ratatui::widgets::Paragraph::new(ratatui::text::Text::from(spans)).block(block), rect); } diff --git a/src/ui/tabs/dashboard.rs b/src/ui/tabs/dashboard.rs index 0c125ab..beb1b28 100644 --- a/src/ui/tabs/dashboard.rs +++ b/src/ui/tabs/dashboard.rs @@ -295,7 +295,7 @@ fn draw_sync_status_card(f: &mut Frame, area: Rect, app: &App) { let name = truncate(&device.name, inner.width.saturating_sub(26).max(4) as usize); let mut spans: Vec = vec![Span::raw(" "), Span::styled(name, Style::default().fg(theme::FG))]; let used: usize = spans.iter().map(|s| s.width()).sum(); - let left = inner.width as usize - 2 - status_span.width(); + let left = (inner.width as usize).saturating_sub(2 + status_span.width()); if used < left { spans.push(Span::raw(" ".repeat(left - used))); } diff --git a/src/ui/tabs/logs.rs b/src/ui/tabs/logs.rs index bc68607..9118cff 100644 --- a/src/ui/tabs/logs.rs +++ b/src/ui/tabs/logs.rs @@ -76,6 +76,7 @@ pub fn draw(f: &mut Frame, area: Rect, app: &App) { .collect(); let content_h = inner.height.saturating_sub(1) as usize; + app.log_view.set(content_h); let offset = if app.log_follow { lines.len().saturating_sub(content_h) } else { diff --git a/src/ui/tabs/sync.rs b/src/ui/tabs/sync.rs index 6b7793d..a9cd09a 100644 --- a/src/ui/tabs/sync.rs +++ b/src/ui/tabs/sync.rs @@ -53,7 +53,7 @@ pub fn draw(f: &mut Frame, area: Rect, app: &App) { } let [steps, progress] = - Layout::vertical([Constraint::Min(6), Constraint::Length(6)]).areas(body); + Layout::vertical([Constraint::Min(6), Constraint::Length(7)]).areas(body); draw_steps(f, steps, &state, app); draw_progress(f, progress, &state); } @@ -202,21 +202,20 @@ fn draw_progress(f: &mut Frame, area: Rect, state: &SyncState) { )); f.render_widget(g, gauge_row); - let mut lines: Vec = Vec::new(); - if !done.is_empty() || !total.is_empty() { - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - if done.is_empty() { "…".to_string() } else { done.clone() }, - Style::default().fg(theme::CYAN), - ), - Span::styled(" / ", Style::default().fg(theme::DIM)), - Span::styled( - if total.is_empty() { "…".to_string() } else { total }, - Style::default().fg(theme::CYAN), - ), - ])); - } + // Line 1: bytes transferred on the left, speed + ETA right-aligned on the + // same row (they used to be pushed onto a third line that the panel height + // clipped, so they were usually invisible). + let mut left: Vec = vec![Span::raw(" ")]; + left.push(Span::styled( + if done.is_empty() { "…".to_string() } else { done.clone() }, + Style::default().fg(theme::CYAN), + )); + left.push(Span::styled(" / ", Style::default().fg(theme::DIM))); + left.push(Span::styled( + if total.is_empty() { "…".to_string() } else { total }, + Style::default().fg(theme::CYAN), + )); + let mut right: Vec = Vec::new(); if !speed.is_empty() { right.push(Span::styled(speed, Style::default().fg(theme::YELLOW))); @@ -225,22 +224,23 @@ fn draw_progress(f: &mut Frame, area: Rect, state: &SyncState) { right.push(Span::styled(" ETA ", Style::default().fg(theme::DIM))); right.push(Span::styled(eta, Style::default().fg(theme::YELLOW))); } - if !current.is_empty() { - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - truncate(¤t, inner.width.saturating_sub(2) as usize), - Style::default().fg(theme::DIM), - ), - ])); - } else { - lines.push(Line::raw("")); - } - let mut stats_line = lines; - stats_line.push(Line::from( - std::iter::once(Span::raw(" ")) - .chain(right.into_iter()) - .collect::>(), - )); - f.render_widget(Paragraph::new(stats_line).style(Style::default().bg(theme::PANEL_BG)), stats); + + let used: usize = left.iter().map(|s| s.width()).sum::() + + right.iter().map(|s| s.width()).sum::(); + let pad = (inner.width as usize).saturating_sub(used + 2); + left.push(Span::raw(" ".repeat(pad))); + left.extend(right); + + let mut lines = vec![Line::from(left)]; + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + truncate(¤t, inner.width.saturating_sub(2) as usize), + Style::default().fg(theme::DIM), + ), + ])); + f.render_widget( + Paragraph::new(lines).style(Style::default().bg(theme::PANEL_BG)), + stats, + ); }