diff --git a/docs/PROBLEMS.md b/docs/PROBLEMS.md index ea6d396..ee71ffb 100644 --- a/docs/PROBLEMS.md +++ b/docs/PROBLEMS.md @@ -7,11 +7,10 @@ ## Table of Contents 1. [Executive Summary](#executive-summary) -2. [Critical Issues (Block Release)](#critical-issues-block-release) -3. [High-Priority Issues (Fix Before v1.0)](#high-priority-issues-fix-before-v10) -4. [Medium-Priority Issues (Follow-Up PRs)](#medium-priority-issues-follow-up-prs) -5. [Low-Priority Issues (Optional)](#low-priority-issues-optional) -6. [Epic Issues / Feature Gaps](#epic-issues--feature-gaps) +2. [High-Priority Issues (Fix Before v1.0)](#high-priority-issues-fix-before-v10) +3. [Medium-Priority Issues (Follow-Up PRs)](#medium-priority-issues-follow-up-prs) +4. [Low-Priority Issues (Optional)](#low-priority-issues-optional) +5. [Epic Issues / Feature Gaps](#epic-issues--feature-gaps) --- @@ -27,253 +26,6 @@ Beyond code quality issues, there are **6 major feature gaps** from PROBLEMS.md --- -## Critical Issues (Block Release) - -### 🔴 CRITICAL #1: Windows Compilation Broken - -**Status:** ✅ SOLVED -**Location:** `src/scanner/dir.rs:181-185` -**Severity:** CRITICAL -**Effort:** 30 minutes - -**Problem:** - -The `get_inode()` function uses Unix-only APIs: -```rust -fn get_inode(path: &Path) -> std::io::Result { - use std::os::unix::fs::MetadataExt; // ← NOT AVAILABLE ON WINDOWS - let metadata = std::fs::metadata(path)?; - Ok(metadata.ino()) -} -``` - -**Solution:** - -Added conditional compilation to provide a stable, cross-platform implementation. On Unix, it continues to use inodes. On Windows and other platforms, it returns an error, causing the scanner to gracefully skip inode-based deduplication and rely on path-based deduplication (which is already implemented via `discovered_prefixes`). - -```rust -#[cfg(unix)] -fn get_inode(path: &Path) -> std::io::Result { - use std::os::unix::fs::MetadataExt; - let metadata = std::fs::metadata(path)?; - Ok(metadata.ino()) -} - -#[cfg(not(unix))] -fn get_inode(_path: &Path) -> std::io::Result { - // On Windows, we can't use inodes. Return error or fallback to path-based dedup. - Err(std::io::Error::new( - std::io::ErrorKind::Other, - "inode tracking not supported on this platform", - )) -} -``` - ---- - -### 🔴 CRITICAL #2: Ecosystem Filtering Logic Mismatch - -**Status:** ✅ SOLVED -**Location:** `src/ui/tui.rs:115-127` vs `src/args.rs:94-110` -**Severity:** CRITICAL -**Effort:** 15 minutes - -**Problem:** - -The TUI background scan thread reimplements ecosystem filtering logic, creating code duplication and potential divergence in behavior. - -**Solution:** - -Refactored `spawn_scan_thread` in `src/ui/tui.rs` to clone the `Args` struct and call its existing `get_ecosystems()` method. This ensures that filtering logic is unified and consistent across both the TUI and plain-text modes. - -```rust -// In src/ui/tui.rs -let target_ecosystems = args_clone.get_ecosystems(&ecosystems); -``` - ---- - -## High-Priority Issues (Fix Before v1.0) - -### 🟠 HIGH #3: Error Handling Divergence - -**Status:** ❌ UNSOLVED -**Location:** `src/lib.rs:13-20` -**Severity:** HIGH -**Effort:** 20 minutes - -**Problem:** - -The main `run()` function has inconsistent error handling: -```rust -pub fn run() { - let args = Args::parse(); - if args.no_tui { - run_plain(args); // ← Silently swallows errors - } else { - ui::tui::run_tui(args).expect("TUI failed"); // ← Can panic! - } -} -``` - -- TUI mode can panic with generic "TUI failed" message (no context) -- Plain-text mode silently swallows errors inside `run_plain()` -- `run()` itself doesn't return a Result, so callers can't handle errors gracefully - -**Example failures:** -- Corrupt ecosystem JSON → unhelpful panic -- Permission denied during deletion → silent failure -- Terminal setup fails → cryptic panic - -**Recommended Fix:** - -Make `run()` return `Result<()>` and propagate errors: -```rust -pub fn run() -> anyhow::Result<()> { - let args = Args::parse(); - if args.no_tui { - run_plain(args) - } else { - ui::tui::run_tui(args) - } -} - -pub fn run_plain(args: Args) -> anyhow::Result<()> { - let ecosystems = config::load_ecosystems() - .context("Failed to load ecosystem configurations")?; - // ... rest of function ... - Ok(()) -} - -// In main.rs: -fn main() { - if let Err(e) = everykill::run() { - eprintln!("Error: {}", e); - std::process::exit(1); - } -} -``` - ---- - -### 🟠 HIGH #4: Dangerous Panic Hook Re-wrapping - -**Status:** ❌ UNSOLVED -**Location:** `src/ui/tui.rs:42-47` -**Severity:** HIGH -**Effort:** 20 minutes - -**Problem:** - -Panic hook is set unconditionally, risking double-wrapping on multiple invocations: - -```rust -let original_hook = std::panic::take_hook(); -std::panic::set_hook(Box::new(move |info| { - let _ = restore_terminal_raw(); - original_hook(info); -})); -``` - -**Risks:** -- If called twice, the second invocation wraps the already-wrapped hook (nesting) -- If wrapped hook panics, recursion occurs -- Terminal might not be restored if panic hook itself panics - -**Recommended Fix:** - -Use atomic flag to prevent re-wrapping: -```rust -use std::sync::atomic::{AtomicBool, Ordering}; - -static PANIC_HOOK_SET: AtomicBool = AtomicBool::new(false); - -pub fn run_tui(args: Args) -> anyhow::Result<()> { - // Only install panic hook once - if !PANIC_HOOK_SET.swap(true, Ordering::SeqCst) { - let original_hook = std::panic::take_hook(); - std::panic::set_hook(Box::new(move |info| { - let _ = restore_terminal_raw(); - original_hook(info); - })); - } - - // Rest of function... -} -``` - ---- - -### 🟠 HIGH #5: Status Messages Cleared Immediately - -**Status:** ❌ UNSOLVED -**Location:** `src/ui/tui.rs:281-283` -**Severity:** HIGH -**Effort:** 30 minutes - -**Problem:** - -Every keystroke immediately clears the status message: -```rust -fn handle_key(state: &mut AppState, key: KeyCode, viewport_height: usize) -> bool { - state.status_message = None; // ← CLEARED ON EVERY KEY! - match &state.mode { ... } -} -``` - -**What happens:** -1. User deletes folders → status shows "Deleted 5 folders, freed 250MB" -2. User presses any key (even arrow) → status is IMMEDIATELY cleared -3. User can't read the completion message - -**Expected behavior:** -- Status persists for 1-2 seconds then auto-clears -- User has time to read results - -**Recommended Fix:** - -Implement timeout-based clearing with timestamp: -```rust -use std::time::Instant; - -pub struct AppState { - pub status_message: Option, - pub status_message_timestamp: Option, // Add this - // ... rest of fields ... -} - -impl AppState { - pub fn show_status(&mut self, message: String) { - self.status_message = Some(message); - self.status_message_timestamp = Some(Instant::now()); - } - - pub fn clear_expired_status(&mut self) { - if let Some(timestamp) = self.status_message_timestamp { - if timestamp.elapsed() > Duration::from_secs(2) { - self.status_message = None; - self.status_message_timestamp = None; - } - } - } -} - -// In handle_key: REMOVE the auto-clear -fn handle_key(state: &mut AppState, key: KeyCode, viewport_height: usize) -> bool { - // REMOVED: state.status_message = None; - match &state.mode { ... } -} - -// In render loop: auto-clear after 2 seconds -while !should_quit { - // ... handle events ... - state.clear_expired_status(); - terminal.draw(|frame| { ... })?; -} -``` - ---- - ## Medium-Priority Issues (Follow-Up PRs) ### 🟡 MEDIUM #6: Integer Overflow in Size Calculation diff --git a/src/lib.rs b/src/lib.rs index 7295e80..684b533 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,23 +5,25 @@ pub mod scanner; pub mod size_util; pub mod ui; +use anyhow::Context; use args::Args; use clap::Parser; use deleter::{delete_folders, print_delete_summary}; use size_util::format_size; -pub fn run() { +pub fn run() -> anyhow::Result<()> { let args = Args::parse(); if args.no_tui { - run_plain(args); + run_plain(args) } else { - ui::tui::run_tui(args).expect("TUI failed"); + ui::tui::run_tui(args) } } -pub fn run_plain(args: Args) { - let ecosystems = config::load_ecosystems().expect("failed to load ecosystems"); +pub fn run_plain(args: Args) -> anyhow::Result<()> { + let ecosystems = + config::load_ecosystems().context("Failed to load ecosystem configurations")?; let target_ecosystems = args.get_ecosystems(&ecosystems); let include_globals = args.should_include_globals(); let excluded_dirs = args.get_excluded_dirs(); @@ -41,7 +43,7 @@ pub fn run_plain(args: Args) { let folders = match args.sort { Some(args::SortBy::Size) => { let mut folders = folders; - folders.sort_by(|a, b| b.size_bytes.cmp(&a.size_bytes)); + folders.sort_by_key(|b| std::cmp::Reverse(b.size_bytes)); folders } Some(args::SortBy::Path) => { @@ -80,4 +82,6 @@ pub fn run_plain(args: Args) { ); } } + + Ok(()) } diff --git a/src/main.rs b/src/main.rs index e74a28b..9452309 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,6 @@ fn main() { - everykill::run(); + if let Err(e) = everykill::run() { + eprintln!("Error: {}", e); + std::process::exit(1); + } } diff --git a/src/ui/app.rs b/src/ui/app.rs index 75b59c3..3b2e019 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1,5 +1,6 @@ use std::collections::HashSet; use std::path::PathBuf; +use std::time::{Duration, Instant}; use crate::config::DiscoveredFolder; @@ -66,6 +67,8 @@ pub struct AppState { /// Transient status message shown in footer (errors, dry-run output, etc.) pub status_message: Option, + /// Timestamp when status_message was set (for auto-clear) + pub status_message_timestamp: Option, } impl AppState { @@ -83,6 +86,21 @@ impl AppState { total_selected_bytes: 0, selected_count: 0, status_message: None, + status_message_timestamp: None, + } + } + + pub fn show_status(&mut self, message: String) { + self.status_message = Some(message); + self.status_message_timestamp = Some(Instant::now()); + } + + pub fn clear_expired_status(&mut self) { + if let Some(timestamp) = self.status_message_timestamp { + if timestamp.elapsed() > Duration::from_secs(5) { + self.status_message = None; + self.status_message_timestamp = None; + } } } diff --git a/src/ui/tui.rs b/src/ui/tui.rs index a148c6c..2257174 100644 --- a/src/ui/tui.rs +++ b/src/ui/tui.rs @@ -1,5 +1,6 @@ use std::collections::HashSet; use std::io::{self, Stdout}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{self, Receiver}; use std::thread; use std::time::{Duration, Instant}; @@ -34,17 +35,23 @@ use crate::ui::widgets::{ // Tick rate for the event loop (~60 fps) const TICK_MS: u64 = 16; +// Guard to prevent double-wrapping of panic hook +static PANIC_HOOK_SET: AtomicBool = AtomicBool::new(false); + // --------------------------------------------------------------------------- // Public entry point // --------------------------------------------------------------------------- pub fn run_tui(args: Args) -> anyhow::Result<()> { // Install panic hook to restore terminal before printing panic details - let original_hook = std::panic::take_hook(); - std::panic::set_hook(Box::new(move |info| { - let _ = restore_terminal_raw(); - original_hook(info); - })); + // Use atomic flag to prevent re-wrapping on multiple invocations + if !PANIC_HOOK_SET.swap(true, Ordering::SeqCst) { + let original_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + let _ = restore_terminal_raw(); + original_hook(info); + })); + } let mut terminal = setup_terminal()?; let result = run_app(&mut terminal, args); @@ -162,6 +169,9 @@ fn run_app(terminal: &mut Terminal>, args: Args) -> any state.handle_scan_event(event); } + // Clear expired status messages + state.clear_expired_status(); + // Render let term_size = terminal.size().unwrap_or_default(); let term_width = term_size.width; @@ -259,9 +269,6 @@ fn render( // --------------------------------------------------------------------------- fn handle_key(state: &mut AppState, key: KeyCode, viewport_height: usize) -> bool { - // Clear transient status message on any key - state.status_message = None; - match &state.mode { AppMode::Normal => handle_key_normal(state, key, viewport_height), AppMode::FilterPopup => { @@ -295,7 +302,7 @@ fn handle_key_normal(state: &mut AppState, key: KeyCode, viewport_height: usize) } else { "Dry-run OFF — deletions are permanent".to_string() }; - state.status_message = Some(msg); + state.show_status(msg); } // Filter popup @@ -355,7 +362,7 @@ fn execute_deletion(state: &mut AppState) { let summary = delete_folders(&state.folders, state.dry_run); if state.dry_run { - state.status_message = Some(format!( + state.show_status(format!( "Dry-run: would free {} from {} folder(s)", crate::size_util::format_size(summary.freed_bytes), summary.deleted_count @@ -383,7 +390,7 @@ fn execute_deletion(state: &mut AppState) { summary.errors.len() ) }; - state.status_message = Some(msg); + state.show_status(msg); } }