refactor: code quality improvements from deep review

- Add error logging in history.rs instead of silent failures
- Extract apply_selection_style() helper and UI style constants
- Remove unused _visible_height parameter from mouse scroll methods
- Fix wrap_content to not add redundant continuation indent
- Remove event cloning in main loop (pre-compute has_start_filter)
- Add MAX_HISTORY_ENTRIES constant
- Add ROADMAP.md to repository

Author: raaymax

.gitignore

@@ -1,7 +1,6 @@
 /target
 CLAUDE.md
 AGENTS.md
-ROADMAP.md
 *.log
 
 # AUR/makepkg build artifacts

ROADMAP.md

@@ -4,6 +4,9 @@ use crate::tab::TabState;
 #[cfg(test)]
 use std::path::PathBuf;
 
+/// Maximum number of filter history entries to keep
+const MAX_HISTORY_ENTRIES: usize = 50;
+
 /// Represents the current view mode
 #[derive(Debug, Clone, PartialEq)]
 pub enum ViewMode {
@@ -160,14 +163,13 @@ impl App {
     }
 
     /// Mouse scroll down
-    pub fn mouse_scroll_down(&mut self, lines: usize, visible_height: usize) {
-        self.active_tab_mut()
-            .mouse_scroll_down(lines, visible_height);
+    pub fn mouse_scroll_down(&mut self, lines: usize) {
+        self.active_tab_mut().mouse_scroll_down(lines);
     }
 
     /// Mouse scroll up
-    pub fn mouse_scroll_up(&mut self, lines: usize, visible_height: usize) {
-        self.active_tab_mut().mouse_scroll_up(lines, visible_height);
+    pub fn mouse_scroll_up(&mut self, lines: usize) {
+        self.active_tab_mut().mouse_scroll_up(lines);
     }
 
     /// Apply filter results
@@ -227,8 +229,8 @@ impl App {
         // Add to history
         self.filter_history.push(entry);
 
-        // Limit history to 50 entries
-        if self.filter_history.len() > 50 {
+        // Limit history size
+        if self.filter_history.len() > MAX_HISTORY_ENTRIES {
             self.filter_history.remove(0);
         }
 

src/app.rs

@@ -26,15 +26,15 @@ impl Default for FilterMode {
 
 impl FilterMode {
     /// Create a new plain text filter mode (case-insensitive by default)
-    #[cfg(test)]
+    #[allow(dead_code)] // Public API for external use and tests
     pub fn plain() -> Self {
         FilterMode::Plain {
             case_sensitive: false,
         }
     }
 
     /// Create a new regex filter mode (case-insensitive by default)
-    #[cfg(test)]
+    #[allow(dead_code)] // Public API for external use and tests
     pub fn regex() -> Self {
         FilterMode::Regex {
             case_sensitive: false,

src/filter/mod.rs

@@ -31,11 +31,21 @@ pub fn save_history(history: &[FilterHistoryEntry]) {
 
     // Create parent directory if it doesn't exist
     if let Some(parent) = path.parent() {
-        let _ = fs::create_dir_all(parent);
+        if let Err(e) = fs::create_dir_all(parent) {
+            eprintln!("Failed to create config directory: {}", e);
+            return;
+        }
     }
 
-    if let Ok(content) = serde_json::to_string_pretty(history) {
-        let _ = fs::write(&path, content);
+    match serde_json::to_string_pretty(history) {
+        Ok(content) => {
+            if let Err(e) = fs::write(&path, content) {
+                eprintln!("Failed to save filter history: {}", e);
+            }
+        }
+        Err(e) => {
+            eprintln!("Failed to serialize filter history: {}", e);
+        }
     }
 }
 

src/history.rs

@@ -368,8 +368,13 @@ fn run_app<B: ratatui::backend::Backend>(terminal: &mut Terminal<B>, app: &mut A
             }
         }
 
+        // Pre-compute if there's a StartFilter event (affects FileModified handling)
+        let has_start_filter = events
+            .iter()
+            .any(|e| matches!(e, AppEvent::StartFilter { .. }));
+
         // Process all collected events
-        for event in events.clone() {
+        for event in events {
             // Handle special events that need side effects (like starting filters)
             match &event {
                 AppEvent::StartFilter {
@@ -398,7 +403,7 @@ fn run_app<B: ratatui::backend::Backend>(terminal: &mut Terminal<B>, app: &mut A
                 | AppEvent::ToggleFilterMode
                 | AppEvent::ToggleCaseSensitivity => {
                     // Apply the event first
-                    app.apply_event(event.clone());
+                    app.apply_event(event);
 
                     // Then trigger live filter preview
                     let pattern = app.get_input().to_string();
@@ -416,15 +421,11 @@ fn run_app<B: ratatui::backend::Backend>(terminal: &mut Terminal<B>, app: &mut A
                     continue; // Event already applied above
                 }
                 AppEvent::MouseScrollDown(lines) => {
-                    // Calculate visible height from terminal size
-                    let visible_height = terminal.size()?.height as usize - PAGE_SIZE_OFFSET - 1;
-                    app.mouse_scroll_down(*lines, visible_height);
+                    app.mouse_scroll_down(*lines);
                     continue; // Event already applied
                 }
                 AppEvent::MouseScrollUp(lines) => {
-                    // Calculate visible height from terminal size
-                    let visible_height = terminal.size()?.height as usize - PAGE_SIZE_OFFSET - 1;
-                    app.mouse_scroll_up(*lines, visible_height);
+                    app.mouse_scroll_up(*lines);
                     continue; // Event already applied
                 }
                 AppEvent::ClearFilter => {
@@ -438,7 +439,7 @@ fn run_app<B: ratatui::backend::Backend>(terminal: &mut Terminal<B>, app: &mut A
                 }
                 AppEvent::FilterComplete { .. } => {
                     // Apply the event first
-                    app.apply_event(event.clone());
+                    app.apply_event(event);
 
                     // Then handle follow mode jump
                     if app.active_tab().follow_mode {
@@ -450,11 +451,9 @@ fn run_app<B: ratatui::backend::Backend>(terminal: &mut Terminal<B>, app: &mut A
                     // For file events, check if we need to jump to end in follow mode
                     let should_jump_follow = app.active_tab().follow_mode
                         && app.active_tab().mode == ViewMode::Normal
-                        && !events
-                            .iter()
-                            .any(|e| matches!(e, AppEvent::StartFilter { .. }));
+                        && !has_start_filter;
 
-                    app.apply_event(event.clone());
+                    app.apply_event(event);
 
                     if should_jump_follow {
                         app.jump_to_end();

src/main.rs

@@ -208,14 +208,14 @@ impl TabState {
     }
 
     /// Mouse scroll down - moves viewport and selection together
-    pub fn mouse_scroll_down(&mut self, lines: usize, _visible_height: usize) {
+    pub fn mouse_scroll_down(&mut self, lines: usize) {
         self.viewport
             .scroll_with_selection(lines as i32, &self.line_indices);
         self.sync_from_viewport();
     }
 
     /// Mouse scroll up - moves viewport and selection together
-    pub fn mouse_scroll_up(&mut self, lines: usize, _visible_height: usize) {
+    pub fn mouse_scroll_up(&mut self, lines: usize) {
         self.viewport
             .scroll_with_selection(-(lines as i32), &self.line_indices);
         self.sync_from_viewport();
@@ -552,11 +552,11 @@ mod tests {
         assert_eq!(tab.selected_line, 5);
 
         // Mouse scroll down - moves both viewport and selection together
-        tab.mouse_scroll_down(3, 20);
+        tab.mouse_scroll_down(3);
         assert_eq!(tab.selected_line, 8); // 5 + 3
 
         // Mouse scroll up
-        tab.mouse_scroll_up(2, 20);
+        tab.mouse_scroll_up(2);
         assert_eq!(tab.selected_line, 6); // 8 - 2
     }
 

src/tab.rs

@@ -10,6 +10,21 @@ use ratatui::{
 };
 use unicode_width::UnicodeWidthStr;
 
+// UI Style Constants
+const SELECTED_BG: Color = Color::DarkGray;
+const EXPANDED_BG: Color = Color::Rgb(30, 30, 40);
+const LINE_PREFIX_WIDTH: usize = 9; // "{:6} | " = 9 characters
+
+/// Apply selection styling to a span (dark bg, bold, adjust dark foreground colors)
+fn apply_selection_style(style: Style) -> Style {
+    // Adjust foreground if it's too dark to see against DarkGray background
+    let adjusted = match style.fg {
+        Some(Color::Gray) | Some(Color::DarkGray) | Some(Color::Black) => style.fg(Color::White),
+        _ => style,
+    };
+    adjusted.bg(SELECTED_BG).add_modifier(Modifier::BOLD)
+}
+
 /// Expand tabs to spaces for proper rendering
 fn expand_tabs(line: &str) -> String {
     if !line.contains('\t') {
@@ -204,10 +219,7 @@ fn render_log_view(f: &mut Frame, area: Rect, app: &mut App) -> Result<()> {
 
     // Calculate available width for content (accounting for borders and line prefix)
     let available_width = area.width.saturating_sub(2) as usize; // Account for borders
-    let prefix_width = 9; // "{:6} | " = 9 characters
-
-    // Background color for expanded (non-selected) entries
-    let expanded_bg = Color::Rgb(30, 30, 40);
+    let prefix_width = LINE_PREFIX_WIDTH;
 
     // Get reader access and collect expanded_lines snapshot
     let mut reader_guard = tab.reader.lock().unwrap();
@@ -228,7 +240,7 @@ fn render_log_view(f: &mut Frame, area: Rect, app: &mut App) -> Result<()> {
             if is_expanded && available_width > prefix_width {
                 // Expanded: wrap content across multiple lines
                 let content_width = available_width.saturating_sub(prefix_width);
-                let wrapped_lines = wrap_content(&line_text, content_width, prefix_width);
+                let wrapped_lines = wrap_content(&line_text, content_width);
 
                 let mut item_lines: Vec<Line<'static>> = Vec::new();
 
@@ -246,17 +258,10 @@ fn render_log_view(f: &mut Frame, area: Rect, app: &mut App) -> Result<()> {
                     // Apply styling based on selection/expansion state
                     for span in &mut wrapped_line.spans {
                         if is_selected {
-                            // Selected: dark gray background + bold
-                            let new_style = match span.style.fg {
-                                Some(Color::Gray) | Some(Color::DarkGray) | Some(Color::Black) => {
-                                    span.style.fg(Color::White)
-                                }
-                                _ => span.style,
-                            };
-                            span.style = new_style.bg(Color::DarkGray).add_modifier(Modifier::BOLD);
+                            span.style = apply_selection_style(span.style);
                         } else {
                             // Expanded but not selected: subtle dark background
-                            span.style = span.style.bg(expanded_bg);
+                            span.style = span.style.bg(EXPANDED_BG);
                         }
                     }
 
@@ -283,13 +288,7 @@ fn render_log_view(f: &mut Frame, area: Rect, app: &mut App) -> Result<()> {
                 // Apply selection background if this is the selected line
                 if is_selected {
                     for span in &mut final_line.spans {
-                        let new_style = match span.style.fg {
-                            Some(Color::Gray) | Some(Color::DarkGray) | Some(Color::Black) => {
-                                span.style.fg(Color::White)
-                            }
-                            _ => span.style,
-                        };
-                        span.style = new_style.bg(Color::DarkGray).add_modifier(Modifier::BOLD);
+                        span.style = apply_selection_style(span.style);
                     }
                 }
 
@@ -427,8 +426,7 @@ fn render_line_jump_prompt(f: &mut Frame, area: Rect, app: &App) {
 /// # Arguments
 /// * `content` - The raw content string (may contain ANSI codes)
 /// * `available_width` - The width in columns to wrap to
-/// * `prefix_width` - The width of the line number prefix (for continuation indent)
-fn wrap_content(content: &str, available_width: usize, prefix_width: usize) -> Vec<Line<'static>> {
+fn wrap_content(content: &str, available_width: usize) -> Vec<Line<'static>> {
     if available_width == 0 {
         return vec![Line::default()];
     }
@@ -456,10 +454,10 @@ fn wrap_content(content: &str, available_width: usize, prefix_width: usize) -> V
     }
 
     // Word wrap the spans
+    // Note: We don't add continuation indent here - the caller (render_log_view) handles prefixes
     let mut result_lines: Vec<Line<'static>> = Vec::new();
     let mut current_line_spans: Vec<Span<'static>> = Vec::new();
     let mut current_line_width = 0;
-    let continuation_indent = " ".repeat(prefix_width);
 
     for span in spans {
         let span_text = span.content.to_string();
@@ -518,9 +516,8 @@ fn wrap_content(content: &str, available_width: usize, prefix_width: usize) -> V
                 // Commit current line and start new one
                 if !current_line_spans.is_empty() {
                     result_lines.push(Line::from(current_line_spans));
-                    // Add continuation indent for wrapped lines
-                    current_line_spans = vec![Span::raw(continuation_indent.clone())];
-                    current_line_width = prefix_width;
+                    current_line_spans = Vec::new();
+                    current_line_width = 0;
                 }
             }
         }