Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
256 changes: 4 additions & 252 deletions docs/PROBLEMS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

---

Expand All @@ -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<u64> {
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<u64> {
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<u64> {
// 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<String>,
pub status_message_timestamp: Option<Instant>, // 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
Expand Down
16 changes: 10 additions & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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) => {
Expand Down Expand Up @@ -80,4 +82,6 @@ pub fn run_plain(args: Args) {
);
}
}

Ok(())
}
5 changes: 4 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
fn main() {
everykill::run();
if let Err(e) = everykill::run() {
eprintln!("Error: {}", e);
std::process::exit(1);
}
}
18 changes: 18 additions & 0 deletions src/ui/app.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::HashSet;
use std::path::PathBuf;
use std::time::{Duration, Instant};

use crate::config::DiscoveredFolder;

Expand Down Expand Up @@ -66,6 +67,8 @@ pub struct AppState {

/// Transient status message shown in footer (errors, dry-run output, etc.)
pub status_message: Option<String>,
/// Timestamp when status_message was set (for auto-clear)
pub status_message_timestamp: Option<Instant>,
}

impl AppState {
Expand All @@ -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;
}
}
}

Expand Down
Loading
Loading