From a6d491d3c245d4191e0b2b8ff920a69ed0240750 Mon Sep 17 00:00:00 2001
From: Arnaud_Cayrol <cayrol.arnaud@gmail.com>
Date: Sat, 31 Jan 2026 09:42:24 +0100
Subject: [PATCH] Added settigns validation, made skip reason more expansible

---
 frontend/src/App.svelte                       |  32 +++++
 .../src/lib/components/PeopleSelector.svelte  |  18 ++-
 .../src/lib/components/ProgressDisplay.svelte |  50 ++++----
 src/config.rs                                 |  61 +++++++++
 src/job/mod.rs                                |  69 ++--------
 src/web/handlers/config.rs                    | 120 +++++++++++++++++-
 src/web/state.rs                              |  59 +++++++++
 7 files changed, 321 insertions(+), 88 deletions(-)

diff --git a/frontend/src/App.svelte b/frontend/src/App.svelte
index b9de5fb..1d414ba 100644
--- a/frontend/src/App.svelte
+++ b/frontend/src/App.svelte
@@ -9,8 +9,38 @@
   import ResultsView from './lib/components/ResultsView.svelte';
   import SettingsPanel from './lib/components/SettingsPanel.svelte';
 
+  // LocalStorage keys
+  const STORAGE_KEY_PERSON = 'immich-timelapse-selected-person';
+
+  // Load persisted state from localStorage
+  function loadPersistedPersonId() {
+    try {
+      const stored = localStorage.getItem(STORAGE_KEY_PERSON);
+      if (stored) {
+        const parsed = JSON.parse(stored);
+        return parsed?.id || null;
+      }
+    } catch (e) {
+      console.warn('Failed to load persisted person:', e);
+    }
+    return null;
+  }
+
+  function persistSelectedPerson(person) {
+    try {
+      if (person) {
+        localStorage.setItem(STORAGE_KEY_PERSON, JSON.stringify({ id: person.id, name: person.name }));
+      } else {
+        localStorage.removeItem(STORAGE_KEY_PERSON);
+      }
+    } catch (e) {
+      console.warn('Failed to persist person:', e);
+    }
+  }
+
   let connectionOk = $state(false);
   let selectedPerson = $state(null);
+  let initialSelectedPersonId = $state(loadPersistedPersonId());
   let jobStatus = $state('idle');
   let progress = $state({ completed: 0, total: 0, message: '' });
   let pollInterval = $state(null);
@@ -93,6 +123,7 @@
 
   function handlePersonSelect(person) {
     selectedPerson = person;
+    persistSelectedPerson(person);
   }
 
   function handleJobUpdate(data) {
@@ -194,6 +225,7 @@
         <PeopleSelector
           onselect={handlePersonSelect}
           disabled={isJobRunning}
+          initialSelectedId={initialSelectedPersonId}
         />
 
         {#if selectedPerson && !isJobRunning}
diff --git a/frontend/src/lib/components/PeopleSelector.svelte b/frontend/src/lib/components/PeopleSelector.svelte
index bbc2bd5..92b4966 100644
--- a/frontend/src/lib/components/PeopleSelector.svelte
+++ b/frontend/src/lib/components/PeopleSelector.svelte
@@ -1,13 +1,13 @@
 <script>
   import { onMount } from 'svelte';
 
-  let { disabled = false, onselect } = $props();
+  let { disabled = false, onselect, initialSelectedId = null } = $props();
 
   let people = $state([]);
   let loading = $state(true);
   let error = $state(null);
   let searchQuery = $state('');
-  let selectedId = $state(null);
+  let selectedId = $state(initialSelectedId);
 
   let filteredPeople = $derived(
     people.filter(p => {
@@ -24,6 +24,20 @@
       const res = await fetch('/api/people');
       if (!res.ok) throw new Error('Failed to load people');
       people = await res.json();
+
+      // If we have an initial selection, find and notify parent about the full person object
+      if (initialSelectedId && !selectedId) {
+        selectedId = initialSelectedId;
+      }
+      if (selectedId) {
+        const person = people.find(p => p.id === selectedId);
+        if (person) {
+          onselect?.(person);
+        } else {
+          // Person no longer exists, clear selection
+          selectedId = null;
+        }
+      }
     } catch (e) {
       error = e.message;
     } finally {
diff --git a/frontend/src/lib/components/ProgressDisplay.svelte b/frontend/src/lib/components/ProgressDisplay.svelte
index 1a5d9de..1db10c4 100644
--- a/frontend/src/lib/components/ProgressDisplay.svelte
+++ b/frontend/src/lib/components/ProgressDisplay.svelte
@@ -30,36 +30,32 @@
   // Person being processed
   let personDisplay = $derived(progress.person_name || progress.person_id || null);
 
-  // Skip statistics from the backend
-  let skipStats = $derived(progress.skip_stats || {
-    face_too_small: 0,
-    eyes_closed: 0,
-    head_turned: 0,
-    too_dark: 0,
-    too_bright: 0,
-    no_face_detected: 0,
-    download_failed: 0,
-    decode_failed: 0,
-    crop_failed: 0,
-    total: 0,
-  });
+  // Convert snake_case to readable label (e.g., "face_too_small" -> "Face too small")
+  function formatLabel(key) {
+    return key
+      .split('_')
+      .map((word, i) => i === 0 ? word.charAt(0).toUpperCase() + word.slice(1) : word)
+      .join(' ');
+  }
+
+  // Skip statistics from the backend - dynamically handle any keys
+  let skipStats = $derived(progress.skip_stats || {});
+
+  // Calculate total from all numeric values in skip_stats
+  let skipTotal = $derived(
+    Object.values(skipStats).reduce((sum, val) => sum + (typeof val === 'number' ? val : 0), 0)
+  );
 
   // Calculate kept (successful) count: completed - skipped
-  let keptCount = $derived(Math.max(0, progress.completed - skipStats.total));
+  let keptCount = $derived(Math.max(0, progress.completed - skipTotal));
 
-  // Filter to only show non-zero skip reasons
+  // Dynamically build skip reasons from whatever the backend sends
+  // Filter to only show non-zero counts
   let skipReasons = $derived(
-    [
-      { label: 'Face too small', count: skipStats.face_too_small },
-      { label: 'Eyes closed', count: skipStats.eyes_closed },
-      { label: 'Head turned', count: skipStats.head_turned },
-      { label: 'Too dark', count: skipStats.too_dark },
-      { label: 'Too bright', count: skipStats.too_bright },
-      { label: 'No face detected', count: skipStats.no_face_detected },
-      { label: 'Download failed', count: skipStats.download_failed },
-      { label: 'Decode failed', count: skipStats.decode_failed },
-      { label: 'Crop failed', count: skipStats.crop_failed },
-    ].filter(r => r.count > 0)
+    Object.entries(skipStats)
+      .filter(([_, count]) => typeof count === 'number' && count > 0)
+      .map(([key, count]) => ({ label: formatLabel(key), count }))
+      .sort((a, b) => b.count - a.count) // Sort by count descending
   );
 
   async function cancelProcessing() {
@@ -104,7 +100,7 @@
       <div class="skip-header">
         <span class="skip-label">Discarded:Kept</span>
         <span class="skip-totals">
-          <span class="skipped-count">{skipStats.total}</span>
+          <span class="skipped-count">{skipTotal}</span>
           <span class="separator">:</span>
           <span class="kept-count">{keptCount}</span>
         </span>
diff --git a/src/config.rs b/src/config.rs
index e51cce7..034716f 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -145,6 +145,25 @@ impl Default for VideoConfig {
     }
 }
 
+/// Persistable configuration (excludes sensitive API credentials).
+///
+/// This struct contains only the settings that can safely be written to disk.
+/// API credentials should always come from environment variables.
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct PersistableConfig {
+    pub processing: ProcessingConfig,
+    pub video: VideoConfig,
+}
+
+impl From<&Config> for PersistableConfig {
+    fn from(config: &Config) -> Self {
+        Self {
+            processing: config.processing.clone(),
+            video: config.video.clone(),
+        }
+    }
+}
+
 impl Config {
     /// Load configuration from a TOML file.
     pub fn from_file(path: impl AsRef<std::path::Path>) -> crate::error::Result<Self> {
@@ -242,6 +261,18 @@ impl Config {
         }
         Ok(())
     }
+
+    /// Save processing and video configuration to a TOML file.
+    ///
+    /// Only saves `processing` and `video` sections - API credentials are
+    /// intentionally excluded as they should come from environment variables.
+    pub fn save_to_file(&self, path: impl AsRef<std::path::Path>) -> crate::error::Result<()> {
+        let persistable = PersistableConfig::from(self);
+        let content = toml::to_string_pretty(&persistable)
+            .map_err(|e| crate::error::Error::Config(format!("Failed to serialize config: {}", e)))?;
+        std::fs::write(path, content)?;
+        Ok(())
+    }
 }
 
 #[cfg(test)]
@@ -262,4 +293,34 @@ mod tests {
         let result = config.validate();
         assert!(result.is_err());
     }
+
+    #[test]
+    fn test_save_and_load_config() {
+        // Create a config with custom values
+        let mut config = Config::default();
+        config.processing.resize_size = 256;
+        config.processing.face_resolution_threshold = 100;
+        config.video.framerate = 30;
+        config.video.crf = 18;
+
+        // Save to a temp file
+        let temp_dir = std::env::temp_dir();
+        let temp_path = temp_dir.join("test_config.toml");
+
+        config.save_to_file(&temp_path).expect("Failed to save config");
+
+        // Verify file was created and contains expected content
+        let content = std::fs::read_to_string(&temp_path).expect("Failed to read config");
+        assert!(content.contains("resize_size = 256"));
+        assert!(content.contains("face_resolution_threshold = 100"));
+        assert!(content.contains("framerate = 30"));
+        assert!(content.contains("crf = 18"));
+
+        // Verify API credentials are NOT in the file
+        assert!(!content.contains("api_key"));
+        assert!(!content.contains("base_url"));
+
+        // Clean up
+        let _ = std::fs::remove_file(&temp_path);
+    }
 }
diff --git a/src/job/mod.rs b/src/job/mod.rs
index c0172b7..46bb1f6 100644
--- a/src/job/mod.rs
+++ b/src/job/mod.rs
@@ -15,7 +15,7 @@ use crate::face_processing::{AssetResult, ProcessedFace, SkipReason};
 use crate::immich_api::{Asset, FaceData, ImmichClient};
 use crate::utils::sanitize_folder_name;
 use crate::video::compile_timelapse;
-use crate::web::{AppState, JobStatus, Progress, SkipStats};
+use crate::web::{AppState, AtomicSkipStats, JobStatus, Progress, SkipStats};
 
 use image::ImageFormat;
 use std::io::Cursor;
@@ -241,16 +241,8 @@ async fn run_job_inner(
     let config = Arc::new(config);
     let output_dirs = Arc::new(output_dirs);
 
-    // Atomic counters for real-time skip statistics
-    let skip_face_too_small = Arc::new(AtomicU32::new(0));
-    let skip_eyes_closed = Arc::new(AtomicU32::new(0));
-    let skip_head_turned = Arc::new(AtomicU32::new(0));
-    let skip_too_dark = Arc::new(AtomicU32::new(0));
-    let skip_too_bright = Arc::new(AtomicU32::new(0));
-    let skip_no_face = Arc::new(AtomicU32::new(0));
-    let skip_download_failed = Arc::new(AtomicU32::new(0));
-    let skip_decode_failed = Arc::new(AtomicU32::new(0));
-    let skip_crop_failed = Arc::new(AtomicU32::new(0));
+    // Atomic counters for real-time skip statistics (consolidated into single struct)
+    let skip_stats = Arc::new(AtomicSkipStats::new());
 
     let mut handles = Vec::with_capacity(assets_with_faces.len());
 
@@ -275,16 +267,8 @@ async fn run_job_inner(
         let state = state.clone();
         let task_cancel_token = cancel_token.clone();
 
-        // Clone skip counters for this task
-        let skip_face_too_small = skip_face_too_small.clone();
-        let skip_eyes_closed = skip_eyes_closed.clone();
-        let skip_head_turned = skip_head_turned.clone();
-        let skip_too_dark = skip_too_dark.clone();
-        let skip_too_bright = skip_too_bright.clone();
-        let skip_no_face = skip_no_face.clone();
-        let skip_download_failed = skip_download_failed.clone();
-        let skip_decode_failed = skip_decode_failed.clone();
-        let skip_crop_failed = skip_crop_failed.clone();
+        // Clone skip stats for this task
+        let skip_stats = skip_stats.clone();
 
         // Clone person info for this task
         let person_id = task_person_id.clone();
@@ -313,34 +297,13 @@ async fn run_job_inner(
 
             // Update skip counters based on result
             if let AssetResult::Skipped { reason, .. } = &result {
-                match reason {
-                    SkipReason::FaceTooSmall => skip_face_too_small.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::EyesClosed => skip_eyes_closed.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::HeadTurned => skip_head_turned.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::TooDark => skip_too_dark.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::TooBright => skip_too_bright.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::NoFaceDetected => skip_no_face.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::DownloadFailed => skip_download_failed.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::DecodeFailed => skip_decode_failed.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::CropFailed => skip_crop_failed.fetch_add(1, Ordering::SeqCst),
-                    SkipReason::Cancelled => 0, // Don't count cancellation
-                };
+                skip_stats.increment(reason);
             }
 
             // Update progress with current skip stats (only if not cancelled)
             if !task_cancel_token.is_cancelled() {
                 let done = completed.fetch_add(1, Ordering::SeqCst) + 1;
-                let current_skip_stats = SkipStats {
-                    face_too_small: skip_face_too_small.load(Ordering::SeqCst),
-                    eyes_closed: skip_eyes_closed.load(Ordering::SeqCst),
-                    head_turned: skip_head_turned.load(Ordering::SeqCst),
-                    too_dark: skip_too_dark.load(Ordering::SeqCst),
-                    too_bright: skip_too_bright.load(Ordering::SeqCst),
-                    no_face_detected: skip_no_face.load(Ordering::SeqCst),
-                    download_failed: skip_download_failed.load(Ordering::SeqCst),
-                    decode_failed: skip_decode_failed.load(Ordering::SeqCst),
-                    crop_failed: skip_crop_failed.load(Ordering::SeqCst),
-                };
+                let current_skip_stats = skip_stats.snapshot();
                 state
                     .update_progress(Progress {
                         status: JobStatus::Running,
@@ -378,17 +341,7 @@ async fn run_job_inner(
     }
 
     // Get final skip statistics from atomic counters
-    let skip_stats = SkipStats {
-        face_too_small: skip_face_too_small.load(Ordering::SeqCst),
-        eyes_closed: skip_eyes_closed.load(Ordering::SeqCst),
-        head_turned: skip_head_turned.load(Ordering::SeqCst),
-        too_dark: skip_too_dark.load(Ordering::SeqCst),
-        too_bright: skip_too_bright.load(Ordering::SeqCst),
-        no_face_detected: skip_no_face.load(Ordering::SeqCst),
-        download_failed: skip_download_failed.load(Ordering::SeqCst),
-        decode_failed: skip_decode_failed.load(Ordering::SeqCst),
-        crop_failed: skip_crop_failed.load(Ordering::SeqCst),
-    };
+    let final_skip_stats = skip_stats.snapshot();
 
     // Count results
     let successful = results
@@ -399,7 +352,7 @@ async fn run_job_inner(
         .iter()
         .filter(|r| matches!(r, AssetResult::Error { .. }))
         .count();
-    let skipped = skip_stats.total();
+    let skipped = final_skip_stats.total();
 
     tracing::info!(
         "Processing complete: {} successful, {} skipped, {} errors",
@@ -415,7 +368,7 @@ async fn run_job_inner(
             completed: total,
             total,
             message: Some("Processing complete, preparing video...".to_string()),
-            skip_stats: skip_stats.clone(),
+            skip_stats: final_skip_stats.clone(),
             person_id: Some(params.person_id.clone()),
             person_name: params.person_name.clone(),
         })
@@ -440,7 +393,7 @@ async fn run_job_inner(
                 completed: 0,
                 total: successful as u32,
                 message: Some("Compiling video...".to_string()),
-                skip_stats: skip_stats.clone(),
+                skip_stats: final_skip_stats.clone(),
                 person_id: Some(params.person_id.clone()),
                 person_name: params.person_name.clone(),
             })
diff --git a/src/web/handlers/config.rs b/src/web/handlers/config.rs
index 04990d8..6c912da 100644
--- a/src/web/handlers/config.rs
+++ b/src/web/handlers/config.rs
@@ -9,6 +9,21 @@ use axum::{
 };
 use serde::{Deserialize, Serialize};
 
+/// Validation error with field name and message.
+struct ValidationError {
+    field: &'static str,
+    message: String,
+}
+
+impl ValidationError {
+    fn new(field: &'static str, message: impl Into<String>) -> Self {
+        Self {
+            field,
+            message: message.into(),
+        }
+    }
+}
+
 /// Configuration response (excludes sensitive API credentials).
 #[derive(Serialize)]
 pub struct ConfigResponse {
@@ -53,6 +68,86 @@ pub struct VideoConfigUpdate {
     pub crf: Option<u32>,
 }
 
+/// Validate processing configuration update values.
+fn validate_processing_config(proc: &ProcessingConfigUpdate) -> Result<(), ValidationError> {
+    if let Some(v) = proc.resize_size {
+        if v < 64 || v > 4096 {
+            return Err(ValidationError::new(
+                "processing.resize_size",
+                format!("must be between 64 and 4096, got {}", v),
+            ));
+        }
+    }
+    if let Some(v) = proc.face_resolution_threshold {
+        if v == 0 || v > 1000 {
+            return Err(ValidationError::new(
+                "processing.face_resolution_threshold",
+                format!("must be between 1 and 1000, got {}", v),
+            ));
+        }
+    }
+    if let Some(v) = proc.pose_threshold {
+        if v <= 0.0 || v > 90.0 {
+            return Err(ValidationError::new(
+                "processing.pose_threshold",
+                format!("must be between 0 and 90 degrees, got {}", v),
+            ));
+        }
+    }
+    if let Some(v) = proc.ear_threshold {
+        if v <= 0.0 || v > 1.0 {
+            return Err(ValidationError::new(
+                "processing.ear_threshold",
+                format!("must be between 0 and 1, got {}", v),
+            ));
+        }
+    }
+    if let Some(v) = proc.max_workers {
+        if v == 0 || v > 64 {
+            return Err(ValidationError::new(
+                "processing.max_workers",
+                format!("must be between 1 and 64, got {}", v),
+            ));
+        }
+    }
+    Ok(())
+}
+
+/// Validate video configuration update values.
+fn validate_video_config(vid: &VideoConfigUpdate) -> Result<(), ValidationError> {
+    if let Some(v) = vid.framerate {
+        if v == 0 || v > 120 {
+            return Err(ValidationError::new(
+                "video.framerate",
+                format!("must be between 1 and 120, got {}", v),
+            ));
+        }
+    }
+    if let Some(v) = vid.crf {
+        if v > 51 {
+            return Err(ValidationError::new(
+                "video.crf",
+                format!("must be between 0 and 51, got {}", v),
+            ));
+        }
+    }
+    if let Some(ref v) = vid.codec {
+        // Allow common video codecs
+        let valid_codecs = ["libx264", "libx265", "libvpx", "libvpx-vp9", "libaom-av1"];
+        if !valid_codecs.contains(&v.as_str()) {
+            return Err(ValidationError::new(
+                "video.codec",
+                format!(
+                    "must be one of: {}, got '{}'",
+                    valid_codecs.join(", "),
+                    v
+                ),
+            ));
+        }
+    }
+    Ok(())
+}
+
 /// Update configuration.
 pub async fn update_config(
     State(state): State<AppState>,
@@ -69,6 +164,24 @@ pub async fn update_config(
         }
     }
 
+    // Validate input before updating
+    if let Some(ref proc) = update.processing {
+        validate_processing_config(proc).map_err(|e| {
+            (
+                StatusCode::BAD_REQUEST,
+                format!("Invalid {}: {}", e.field, e.message),
+            )
+        })?;
+    }
+    if let Some(ref vid) = update.video {
+        validate_video_config(vid).map_err(|e| {
+            (
+                StatusCode::BAD_REQUEST,
+                format!("Invalid {}: {}", e.field, e.message),
+            )
+        })?;
+    }
+
     // Update config
     {
         let mut config = state.config.write().await;
@@ -109,7 +222,12 @@ pub async fn update_config(
             }
         }
 
-        tracing::info!("Configuration updated");
+        // Persist to file (non-blocking, best-effort)
+        if let Err(e) = config.save_to_file("config.toml") {
+            tracing::warn!("Failed to persist config to file: {}", e);
+        } else {
+            tracing::info!("Configuration updated and persisted to config.toml");
+        }
     }
 
     // Return updated config
diff --git a/src/web/state.rs b/src/web/state.rs
index da73984..a16e697 100644
--- a/src/web/state.rs
+++ b/src/web/state.rs
@@ -1,6 +1,7 @@
 //! Application state for the web server.
 
 use crate::config::Config;
+use std::sync::atomic::{AtomicU32, Ordering};
 use std::sync::Arc;
 use tokio::sync::{broadcast, RwLock};
 use tokio_util::sync::CancellationToken;
@@ -54,6 +55,64 @@ impl SkipStats {
     }
 }
 
+/// Atomic version of SkipStats for thread-safe concurrent updates.
+///
+/// This consolidates all skip counters into a single struct that can be
+/// shared across async tasks with Arc, avoiding the need to clone multiple
+/// individual atomic counters.
+#[derive(Debug, Default)]
+pub struct AtomicSkipStats {
+    pub face_too_small: AtomicU32,
+    pub eyes_closed: AtomicU32,
+    pub head_turned: AtomicU32,
+    pub too_dark: AtomicU32,
+    pub too_bright: AtomicU32,
+    pub no_face_detected: AtomicU32,
+    pub download_failed: AtomicU32,
+    pub decode_failed: AtomicU32,
+    pub crop_failed: AtomicU32,
+}
+
+impl AtomicSkipStats {
+    /// Create a new AtomicSkipStats with all counters at zero.
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    /// Increment a specific counter and return the new value.
+    pub fn increment(&self, reason: &crate::face_processing::SkipReason) -> u32 {
+        use crate::face_processing::SkipReason;
+        let counter = match reason {
+            SkipReason::FaceTooSmall => &self.face_too_small,
+            SkipReason::EyesClosed => &self.eyes_closed,
+            SkipReason::HeadTurned => &self.head_turned,
+            SkipReason::TooDark => &self.too_dark,
+            SkipReason::TooBright => &self.too_bright,
+            SkipReason::NoFaceDetected => &self.no_face_detected,
+            SkipReason::DownloadFailed => &self.download_failed,
+            SkipReason::DecodeFailed => &self.decode_failed,
+            SkipReason::CropFailed => &self.crop_failed,
+            SkipReason::Cancelled => return 0, // Don't count cancellation
+        };
+        counter.fetch_add(1, Ordering::SeqCst) + 1
+    }
+
+    /// Get a snapshot of the current stats as a non-atomic SkipStats.
+    pub fn snapshot(&self) -> SkipStats {
+        SkipStats {
+            face_too_small: self.face_too_small.load(Ordering::SeqCst),
+            eyes_closed: self.eyes_closed.load(Ordering::SeqCst),
+            head_turned: self.head_turned.load(Ordering::SeqCst),
+            too_dark: self.too_dark.load(Ordering::SeqCst),
+            too_bright: self.too_bright.load(Ordering::SeqCst),
+            no_face_detected: self.no_face_detected.load(Ordering::SeqCst),
+            download_failed: self.download_failed.load(Ordering::SeqCst),
+            decode_failed: self.decode_failed.load(Ordering::SeqCst),
+            crop_failed: self.crop_failed.load(Ordering::SeqCst),
+        }
+    }
+}
+
 /// Progress information for a running job.
 #[derive(Debug, Clone)]
 pub struct Progress {