From 8e4f91561b9b797de02fadcb78a9c9a24eb2d767 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sun, 7 Jun 2026 18:28:22 -0400 Subject: [PATCH] Add per-file insight history endpoint and rate-by-id Expose GET /insights/history?path=... returning every generated version of a photo's insight (current plus superseded), newest-first, backing the mobile per-file insight history view. - New get_insight_history_handler; reuses the existing get_insight_history DAO method (removed its dead_code allow). - impl From for PhotoInsightResponse, collapsing the mapping that was duplicated across the single-get and all-insights handlers. - rate_insight_by_id DAO method + optional insight_id on RateInsightRequest so previously generated versions can be approved/rejected (the path-based rate only touches the current row). - DAO tests for history ordering/scoping and id-targeted rating. - cargo fmt normalized a multi-line assert in insight_chat.rs tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/handlers.rs | 122 ++++++++++++++++++++++------------- src/ai/insight_chat.rs | 5 +- src/ai/mod.rs | 3 +- src/database/insights_dao.rs | 119 +++++++++++++++++++++++++++++++++- src/main.rs | 1 + 5 files changed, 201 insertions(+), 49 deletions(-) diff --git a/src/ai/handlers.rs b/src/ai/handlers.rs index 9fbe6b7..9bcb048 100644 --- a/src/ai/handlers.rs +++ b/src/ai/handlers.rs @@ -8,7 +8,7 @@ use crate::ai::insight_chat::{ChatStreamEvent, ChatTurnRequest}; use crate::ai::ollama::ChatMessage; use crate::ai::{ModelCapabilities, OllamaClient}; use crate::data::Claims; -use crate::database::models::{InsightGenerationType, InsightJobStatus}; +use crate::database::models::{InsightGenerationType, InsightJobStatus, PhotoInsight}; use crate::database::{ExifDao, InsightDao}; use crate::libraries; use crate::otel::{extract_context_from_request, global_tracer}; @@ -273,6 +273,11 @@ pub async fn cancel_generation_handler( pub struct RateInsightRequest { pub file_path: String, pub approved: bool, + /// When set, rate this specific insight version by primary key + /// (used by the per-file history view to rate superseded versions). + /// When omitted, the current insight for `file_path` is rated. + #[serde(default)] + pub insight_id: Option, } #[derive(Debug, Deserialize)] @@ -333,6 +338,31 @@ pub struct PhotoInsightResponse { pub persona_id: Option, } +impl From for PhotoInsightResponse { + fn from(insight: PhotoInsight) -> Self { + PhotoInsightResponse { + id: insight.id, + file_path: insight.file_path, + title: insight.title, + summary: insight.summary, + generated_at: insight.generated_at, + model_version: insight.model_version, + prompt_eval_count: insight.prompt_eval_count, + eval_count: insight.eval_count, + approved: insight.approved, + has_training_messages: insight.training_messages.is_some(), + backend: insight.backend, + num_ctx: insight.num_ctx, + temperature: insight.temperature, + top_p: insight.top_p, + top_k: insight.top_k, + min_p: insight.min_p, + system_prompt: insight.system_prompt, + persona_id: insight.persona_id, + } + } +} + #[derive(Debug, Serialize)] pub struct AvailableModelsResponse { pub primary: ServerModels, @@ -554,29 +584,7 @@ pub async fn get_insight_handler( let mut dao = insight_dao.lock().expect("Unable to lock InsightDao"); match dao.get_insight_for_paths(&otel_context, &sibling_paths) { - Ok(Some(insight)) => { - let response = PhotoInsightResponse { - id: insight.id, - file_path: insight.file_path, - title: insight.title, - summary: insight.summary, - generated_at: insight.generated_at, - model_version: insight.model_version, - prompt_eval_count: insight.prompt_eval_count, - eval_count: insight.eval_count, - approved: insight.approved, - has_training_messages: insight.training_messages.is_some(), - backend: insight.backend, - num_ctx: insight.num_ctx, - temperature: insight.temperature, - top_p: insight.top_p, - top_k: insight.top_k, - min_p: insight.min_p, - system_prompt: insight.system_prompt, - persona_id: insight.persona_id, - }; - HttpResponse::Ok().json(response) - } + Ok(Some(insight)) => HttpResponse::Ok().json(PhotoInsightResponse::from(insight)), Ok(None) => HttpResponse::NotFound().json(serde_json::json!({ "error": "Insight not found" })), @@ -631,26 +639,7 @@ pub async fn get_all_insights_handler( Ok(insights) => { let responses: Vec = insights .into_iter() - .map(|insight| PhotoInsightResponse { - id: insight.id, - file_path: insight.file_path, - title: insight.title, - summary: insight.summary, - generated_at: insight.generated_at, - model_version: insight.model_version, - prompt_eval_count: insight.prompt_eval_count, - eval_count: insight.eval_count, - approved: insight.approved, - has_training_messages: insight.training_messages.is_some(), - backend: insight.backend, - num_ctx: insight.num_ctx, - temperature: insight.temperature, - top_p: insight.top_p, - top_k: insight.top_k, - min_p: insight.min_p, - system_prompt: insight.system_prompt, - persona_id: insight.persona_id, - }) + .map(PhotoInsightResponse::from) .collect(); HttpResponse::Ok().json(responses) @@ -664,6 +653,39 @@ pub async fn get_all_insights_handler( } } +/// GET /insights/history?path=/path/to/photo.jpg - Get all insight versions +/// for a single photo (current plus previously generated/superseded ones), +/// newest first. Backs the per-file insight history view. +#[get("/insights/history")] +pub async fn get_insight_history_handler( + _claims: Claims, + query: web::Query, + insight_dao: web::Data>>, +) -> impl Responder { + let normalized_path = normalize_path(&query.path); + log::debug!("Fetching insight history for {}", normalized_path); + + let otel_context = opentelemetry::Context::new(); + let mut dao = insight_dao.lock().expect("Unable to lock InsightDao"); + + match dao.get_insight_history(&otel_context, &normalized_path) { + Ok(insights) => { + let responses: Vec = insights + .into_iter() + .map(PhotoInsightResponse::from) + .collect(); + + HttpResponse::Ok().json(responses) + } + Err(e) => { + log::error!("Failed to fetch insight history ({}): {:?}", &query.path, e); + HttpResponse::InternalServerError().json(serde_json::json!({ + "error": format!("Failed to fetch insight history: {:?}", e) + })) + } + } +} + /// POST /insights/generate/agentic - Generate insight using agentic tool-calling loop (async) #[post("/insights/generate/agentic")] pub async fn generate_agentic_insight_handler( @@ -1012,15 +1034,23 @@ pub async fn rate_insight_handler( ) -> impl Responder { let normalized_path = normalize_path(&request.file_path); log::info!( - "Rating insight for {}: approved={}", + "Rating insight for {} (id={:?}): approved={}", normalized_path, + request.insight_id, request.approved ); let otel_context = opentelemetry::Context::new(); let mut dao = insight_dao.lock().expect("Unable to lock InsightDao"); - match dao.rate_insight(&otel_context, &normalized_path, request.approved) { + // Rate a specific version by id when provided (history view), otherwise + // rate the current insight for the path. + let result = match request.insight_id { + Some(id) => dao.rate_insight_by_id(&otel_context, id, request.approved), + None => dao.rate_insight(&otel_context, &normalized_path, request.approved), + }; + + match result { Ok(()) => HttpResponse::Ok().json(serde_json::json!({ "success": true, "message": "Insight rated successfully" diff --git a/src/ai/insight_chat.rs b/src/ai/insight_chat.rs index f4d478b..3c5eb26 100644 --- a/src/ai/insight_chat.rs +++ b/src/ai/insight_chat.rs @@ -2473,7 +2473,10 @@ mod tests { let original_len = msgs.len(); let dropped = apply_context_budget(&mut msgs, budget_bytes); - assert!(!dropped, "short conversation with one image must not truncate"); + assert!( + !dropped, + "short conversation with one image must not truncate" + ); assert_eq!(msgs.len(), original_len, "no messages should be dropped"); // Sanity: the flat image charge is accounted for but stays well under budget. assert!(estimate_bytes(&msgs) <= budget_bytes); diff --git a/src/ai/mod.rs b/src/ai/mod.rs index e61eace..40a3f21 100644 --- a/src/ai/mod.rs +++ b/src/ai/mod.rs @@ -25,7 +25,8 @@ pub use handlers::{ chat_stream_handler, chat_turn_handler, delete_insight_handler, export_training_data_handler, generate_agentic_insight_handler, generate_insight_handler, generation_status_handler, get_all_insights_handler, get_available_models_handler, get_insight_handler, - get_openrouter_models_handler, rate_insight_handler, turn_async_handler, turn_replay_handler, + get_insight_history_handler, get_openrouter_models_handler, rate_insight_handler, + turn_async_handler, turn_replay_handler, }; pub use insight_generator::InsightGenerator; pub use llamacpp::LlamaCppClient; diff --git a/src/database/insights_dao.rs b/src/database/insights_dao.rs index 6b467ea..3880550 100644 --- a/src/database/insights_dao.rs +++ b/src/database/insights_dao.rs @@ -47,7 +47,6 @@ pub trait InsightDao: Sync + Send { paths: &[String], ) -> Result, DbError>; - #[allow(dead_code)] fn get_insight_history( &mut self, context: &opentelemetry::Context, @@ -82,6 +81,17 @@ pub trait InsightDao: Sync + Send { approved: bool, ) -> Result<(), DbError>; + /// Rate a specific insight version by primary key, regardless of + /// `is_current`. Used by the per-file history view to approve/reject + /// previously generated (superseded) versions, which the path-based + /// `rate_insight` (current row only) cannot reach. + fn rate_insight_by_id( + &mut self, + context: &opentelemetry::Context, + insight_id: i32, + approved: bool, + ) -> Result<(), DbError>; + fn get_approved_insights( &mut self, context: &opentelemetry::Context, @@ -352,6 +362,26 @@ impl InsightDao for SqliteInsightDao { .map_err(|e| DbError::log(DbErrorKind::UpdateError, e)) } + fn rate_insight_by_id( + &mut self, + context: &opentelemetry::Context, + target_id: i32, + is_approved: bool, + ) -> Result<(), DbError> { + trace_db_call(context, "update", "rate_insight_by_id", |_span| { + use schema::photo_insights::dsl::*; + + let mut connection = self.connection.lock().expect("Unable to get InsightDao"); + + diesel::update(photo_insights.find(target_id)) + .set(approved.eq(Some(is_approved))) + .execute(connection.deref_mut()) + .map(|_| ()) + .map_err(|e| anyhow::anyhow!("Update error: {}", e)) + }) + .map_err(|e| DbError::log(DbErrorKind::UpdateError, e)) + } + fn get_approved_insights( &mut self, context: &opentelemetry::Context, @@ -396,3 +426,90 @@ impl InsightDao for SqliteInsightDao { .map_err(|e| DbError::log(DbErrorKind::UpdateError, e)) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::database::test::in_memory_db_connection; + + fn dao() -> SqliteInsightDao { + let conn = Arc::new(Mutex::new(in_memory_db_connection())); + SqliteInsightDao::from_connection(conn) + } + + /// Build an insight insert with sensible defaults; tests override the + /// fields they care about (path, generated_at, model). + fn insert(path: &str, generated_at: i64, model: &str) -> InsertPhotoInsight { + InsertPhotoInsight { + library_id: 1, + file_path: path.to_string(), + title: format!("title for {model}"), + summary: "summary".to_string(), + generated_at, + model_version: model.to_string(), + is_current: true, + training_messages: None, + backend: "local".to_string(), + fewshot_source_ids: None, + content_hash: None, + num_ctx: None, + temperature: None, + top_p: None, + top_k: None, + min_p: None, + system_prompt: None, + persona_id: None, + prompt_eval_count: None, + eval_count: None, + } + } + + #[test] + fn get_insight_history_returns_all_versions_newest_first() { + let cx = opentelemetry::Context::new(); + let mut dao = dao(); + + // store_insight flips prior rows to is_current=false, so three + // generations for the same path leave a 3-row history. + dao.store_insight(&cx, insert("a.jpg", 100, "m1")).unwrap(); + dao.store_insight(&cx, insert("a.jpg", 200, "m2")).unwrap(); + dao.store_insight(&cx, insert("a.jpg", 300, "m3")).unwrap(); + // A different path must not leak into the history. + dao.store_insight(&cx, insert("b.jpg", 250, "other")) + .unwrap(); + + let history = dao.get_insight_history(&cx, "a.jpg").unwrap(); + assert_eq!(history.len(), 3); + assert_eq!( + history.iter().map(|i| i.generated_at).collect::>(), + vec![300, 200, 100], + "history should be newest-first" + ); + // Exactly one version is current (the latest generation). + let current: Vec<_> = history.iter().filter(|i| i.is_current).collect(); + assert_eq!(current.len(), 1); + assert_eq!(current[0].generated_at, 300); + } + + #[test] + fn rate_insight_by_id_rates_only_the_targeted_version() { + let cx = opentelemetry::Context::new(); + let mut dao = dao(); + + dao.store_insight(&cx, insert("a.jpg", 100, "m1")).unwrap(); + dao.store_insight(&cx, insert("a.jpg", 200, "m2")).unwrap(); + + // History is newest-first: [200 (current), 100 (superseded)]. + let history = dao.get_insight_history(&cx, "a.jpg").unwrap(); + let old_version = history.iter().find(|i| i.generated_at == 100).unwrap(); + assert!(!old_version.is_current); + + dao.rate_insight_by_id(&cx, old_version.id, true).unwrap(); + + let history = dao.get_insight_history(&cx, "a.jpg").unwrap(); + let old = history.iter().find(|i| i.generated_at == 100).unwrap(); + let current = history.iter().find(|i| i.generated_at == 200).unwrap(); + assert_eq!(old.approved, Some(true), "targeted version is rated"); + assert_eq!(current.approved, None, "current version is untouched"); + } +} diff --git a/src/main.rs b/src/main.rs index 8b06228..f27cf8f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -351,6 +351,7 @@ fn main() -> std::io::Result<()> { .service(ai::get_insight_handler) .service(ai::delete_insight_handler) .service(ai::get_all_insights_handler) + .service(ai::get_insight_history_handler) .service(ai::get_available_models_handler) .service(ai::get_openrouter_models_handler) .service(ai::chat_turn_handler)