refactor: improve error handling and simplify codebase

Updated the configuration loading to use explicit error results rather than
silent failures. Replaced silent filtering of invalid configuration items with
descriptive errors and performed a code-wide cleanup by utilizing pattern
guards in key-handling logic and implementing atomic file writing for state
files.

Signed-off-by: Chmouel Boudjnah <chmouel@chmouel.com>

Author: chmouel

src/config.rs

@@ -268,8 +268,8 @@ impl Config {
     /// Load configuration from file and environment variables.
     /// Priority: CLI args > env vars > config file > defaults
     /// If config_path is provided, use it instead of the default path.
-    pub fn load(config_path: Option<&Path>) -> Self {
-        let mut config = Self::load_from_file(config_path).unwrap_or_default();
+    pub fn load(config_path: Option<&Path>) -> Result<Self> {
+        let mut config = Self::load_from_file(config_path)?.unwrap_or_default();
 
         // Environment variable overrides for backwards compatibility
         if let Ok(interval) = env::var("GH_NEWS_AUTO_REFRESH_INTERVAL") {
@@ -278,7 +278,7 @@ impl Config {
             }
         }
 
-        config
+        Ok(config)
     }
 
     /// Get the default config file path (~/.config/gh-news/config.toml).
@@ -291,19 +291,40 @@ impl Config {
 
     /// Load configuration from the TOML config file.
     /// If config_path is provided, use it; otherwise use the default path.
-    fn load_from_file(config_path: Option<&Path>) -> Option<Self> {
+    fn load_from_file(config_path: Option<&Path>) -> Result<Option<Self>> {
         let path = match config_path {
             Some(p) => p.to_path_buf(),
-            None => Self::get_config_path().ok()?,
+            None => match Self::get_config_path() {
+                Ok(path) => path,
+                Err(_) => return Ok(None),
+            },
         };
 
         if !path.exists() {
-            return None;
+            if config_path.is_some() {
+                return Err(Error::Config(format!(
+                    "Config file not found: {}",
+                    path.display()
+                )));
+            }
+            return Ok(None);
         }
 
-        let content = fs::read_to_string(&path).ok()?;
-        let config: Config = toml::from_str(&content).ok()?;
-        Some(config)
+        let content = fs::read_to_string(&path).map_err(|e| {
+            Error::Config(format!(
+                "Failed to read config file {}: {}",
+                path.display(),
+                e
+            ))
+        })?;
+        let config: Config = toml::from_str(&content).map_err(|e| {
+            Error::Config(format!(
+                "Failed to parse config file {}: {}",
+                path.display(),
+                e
+            ))
+        })?;
+        Ok(Some(config))
     }
 
     /// Build the colour palette from the configured theme name and any overrides.
@@ -337,6 +358,7 @@ impl Config {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::fs;
 
     #[test]
     fn test_default_config() {
@@ -549,4 +571,34 @@ red = "#112233"
         let default = crate::ui::theme::ColorPalette::tokyo_night();
         assert_eq!(palette.blue, default.blue);
     }
+
+    #[test]
+    fn test_load_reports_missing_explicit_config() {
+        let missing = std::env::temp_dir().join(format!(
+            "gh-news-missing-config-{}-{}.toml",
+            std::process::id(),
+            "missing"
+        ));
+        let err = Config::load(Some(&missing)).unwrap_err();
+        assert!(err
+            .to_string()
+            .contains(&format!("Config file not found: {}", missing.display())));
+    }
+
+    #[test]
+    fn test_load_reports_invalid_config() {
+        let path = std::env::temp_dir().join(format!(
+            "gh-news-invalid-config-{}-{}.toml",
+            std::process::id(),
+            "parse"
+        ));
+        fs::write(&path, "auto_refresh_interval = [").unwrap();
+
+        let err = Config::load(Some(&path)).unwrap_err();
+        assert!(err
+            .to_string()
+            .contains(&format!("Failed to parse config file {}", path.display())));
+
+        let _ = fs::remove_file(path);
+    }
 }

src/events.rs

@@ -133,7 +133,7 @@ pub fn fetch_watch_repo_events(
         }
     }
 
-    all_notifications.sort_by(|a, b| b.updated_at.cmp(&a.updated_at));
+    all_notifications.sort_by_key(|n| std::cmp::Reverse(n.updated_at));
 
     enrich_event_titles(client, &mut all_notifications);
 

src/filter.rs

@@ -1,5 +1,5 @@
 use crate::config::{Config, View};
-use crate::error::Result;
+use crate::error::{Error, Result};
 use crate::models::enums::{NotificationReason, NotificationType};
 use crate::models::Notification;
 use regex::Regex;
@@ -52,37 +52,41 @@ impl Filter {
     ) -> Result<Self> {
         let parsed_types: Vec<NotificationType> = exclude_types
             .iter()
-            .filter_map(|s| {
-                let t: NotificationType = s.parse().ok()?;
-                if t == NotificationType::Unknown {
-                    None
+            .map(|s| {
+                let parsed: NotificationType = s.parse().unwrap_or(NotificationType::Unknown);
+                if parsed == NotificationType::Unknown {
+                    Err(Error::Config(format!(
+                        "Unknown notification type in exclude_types: {s}"
+                    )))
                 } else {
-                    Some(t)
+                    Ok(parsed)
                 }
             })
-            .collect();
+            .collect::<Result<_>>()?;
 
         let parsed_reasons: Vec<NotificationReason> = exclude_reasons
             .iter()
-            .filter_map(|s| {
-                let r: NotificationReason = s.parse().ok()?;
-                if r == NotificationReason::Unknown {
-                    None
+            .map(|s| {
+                let parsed: NotificationReason = s.parse().unwrap_or(NotificationReason::Unknown);
+                if parsed == NotificationReason::Unknown {
+                    Err(Error::Config(format!(
+                        "Unknown notification reason in exclude_reasons: {s}"
+                    )))
                 } else {
-                    Some(r)
+                    Ok(parsed)
                 }
             })
-            .collect();
+            .collect::<Result<_>>()?;
 
         let parsed_repos: Vec<RepoPattern> = exclude_repos
             .iter()
-            .filter_map(|s| RepoPattern::new(s).ok())
-            .collect();
+            .map(|s| RepoPattern::new(s))
+            .collect::<Result<_>>()?;
 
         let parsed_subjects: Vec<Regex> = exclude_subjects
             .iter()
-            .filter_map(|s| Regex::new(&format!("(?i){s}")).ok())
-            .collect();
+            .map(|s| Regex::new(&format!("(?i){s}")).map_err(Error::from))
+            .collect::<Result<_>>()?;
 
         let mut patterns = Vec::new();
         if let Some(pattern) = pattern {
@@ -290,18 +294,19 @@ mod tests {
     }
 
     #[test]
-    fn test_unrecognised_type_ignored() {
-        let filter = Filter::new(
-            None,
-            &["NonExistentType".to_string(), "Issue".to_string()],
-            &[],
-            &[],
-            &[],
-        )
-        .unwrap();
-        // Only Issue should be excluded, unrecognised type silently ignored
-        assert_eq!(filter.exclude_types.len(), 1);
-        assert_eq!(filter.exclude_types[0], NotificationType::Issue);
+    fn test_unrecognised_type_returns_error() {
+        let err = Filter::new(None, &["NonExistentType".to_string()], &[], &[], &[]).unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("Unknown notification type in exclude_types: NonExistentType"));
+    }
+
+    #[test]
+    fn test_unrecognised_reason_returns_error() {
+        let err = Filter::new(None, &[], &["mystery".to_string()], &[], &[]).unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("Unknown notification reason in exclude_reasons: mystery"));
     }
 
     #[test]
@@ -373,4 +378,10 @@ mod tests {
         assert!(!filter.matches(&lower));
         assert!(filter.matches(&normal));
     }
+
+    #[test]
+    fn test_invalid_subject_regex_returns_error() {
+        let err = Filter::new(None, &[], &[], &[], &["[".to_string()]).unwrap_err();
+        assert!(err.to_string().contains("Filter error"));
+    }
 }

src/main.rs

@@ -63,7 +63,7 @@ impl RuntimeOptions {
 
 fn run() -> Result<()> {
     let args = Args::parse();
-    let config = Config::load(args.config.as_deref());
+    let config = Config::load(args.config.as_deref())?;
     let opts = RuntimeOptions::from_args_and_config(&args, &config);
 
     // Initialise state file path: CLI > config > default
@@ -139,38 +139,24 @@ fn run() -> Result<()> {
     app.start_auto_refresh(opts.show_all, opts.participating, opts.max_notifications);
 
     // Apply filters
-    let filter = opts
-        .filter_pattern
-        .as_deref()
-        .map(|pattern| Filter::from_config(Some(pattern), &config))
-        .transpose()?
-        .or_else(|| {
-            // Even without a regex pattern, apply structured excludes if configured
-            if config.exclude_types.is_empty()
-                && config.exclude_reasons.is_empty()
-                && config.exclude_repos.is_empty()
-                && config.exclude_subjects.is_empty()
-            {
-                None
-            } else {
-                Filter::from_config(None, &config).ok()
-            }
-        });
+    let filter = if let Some(pattern) = opts.filter_pattern.as_deref() {
+        Some(Filter::from_config(Some(pattern), &config)?)
+    } else if config.exclude_types.is_empty()
+        && config.exclude_reasons.is_empty()
+        && config.exclude_repos.is_empty()
+        && config.exclude_subjects.is_empty()
+    {
+        None
+    } else {
+        Some(Filter::from_config(None, &config)?)
+    };
 
     // Always use config's default preview mode on startup
     let preview_mode = config.get_default_preview_mode();
 
-    // Config disabling always wins; otherwise use the persisted toggle (m-key).
-    let auto_mark_read = if !config.auto_mark_read {
-        false
-    } else {
-        state_file::AppStateFile::load_auto_mark_read().unwrap_or(false)
-    };
-    let auto_archive = if !config.auto_archive {
-        false
-    } else {
-        state_file::AppStateFile::load_auto_archive().unwrap_or(false)
-    };
+    let auto_mark_read =
+        state_file::AppStateFile::load_auto_mark_read().unwrap_or(config.auto_mark_read);
+    let auto_archive = config.auto_archive;
 
     // Set initial values. `set_auto_archive` will correctly enforce `auto_mark_read` if needed.
     app.set_auto_mark_read(auto_mark_read);

src/state/tree.rs

@@ -129,7 +129,7 @@ impl<'a> TreeBuilder<'a> {
             })
             .collect();
 
-        org_list.sort_by(|a, b| b.2.cmp(&a.2));
+        org_list.sort_by_key(|o| std::cmp::Reverse(o.2));
 
         for (org, indices, _) in org_list {
             let notification_count = indices.len();
@@ -198,7 +198,7 @@ impl<'a> TreeBuilder<'a> {
             })
             .collect();
 
-        repo_list.sort_by(|a, b| b.2.cmp(&a.2));
+        repo_list.sort_by_key(|r| std::cmp::Reverse(r.2));
         repo_list
     }