From 6f0c15d0c5603727b1add557d69fb25bed288a2d Mon Sep 17 00:00:00 2001
From: Cameron Cordes <cameronc.dev@gmail.com>
Date: Thu, 7 May 2026 17:48:22 -0400
Subject: [PATCH] insight-chat: code-review polish on get_faces_in_photo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Drop redundant `use anyhow::Context` inside has_any_faces (already
  imported at the module level).
- Drop dead `.unwrap_or("?")` on bound faces — the vec is filtered to
  is_some() so the fallback can never fire.
- Reorder the face_dao constructor param + initializer to match the
  struct declaration (between tag_dao and knowledge_dao). Update both
  state.rs call sites and populate_knowledge.rs to match.
- Hold face_dao lock once across the library-resolver loop instead of
  reacquiring per iteration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 src/ai/insight_generator.rs   | 25 ++++++++++++++-----------
 src/bin/populate_knowledge.rs |  2 +-
 src/faces.rs                  |  1 -
 src/state.rs                  |  4 ++--
 4 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/ai/insight_generator.rs b/src/ai/insight_generator.rs
index 632b504..1267628 100644
--- a/src/ai/insight_generator.rs
+++ b/src/ai/insight_generator.rs
@@ -119,8 +119,8 @@ impl InsightGenerator {
         location_dao: Arc<Mutex<Box<dyn LocationHistoryDao>>>,
         search_dao: Arc<Mutex<Box<dyn SearchHistoryDao>>>,
         tag_dao: Arc<Mutex<Box<dyn TagDao>>>,
-        knowledge_dao: Arc<Mutex<Box<dyn KnowledgeDao>>>,
         face_dao: Arc<Mutex<Box<dyn crate::faces::FaceDao>>>,
+        knowledge_dao: Arc<Mutex<Box<dyn KnowledgeDao>>>,
         libraries: Vec<Library>,
     ) -> Self {
         Self {
@@ -135,8 +135,8 @@ impl InsightGenerator {
             location_dao,
             search_dao,
             tag_dao,
-            knowledge_dao,
             face_dao,
+            knowledge_dao,
             libraries,
         }
     }
@@ -2180,15 +2180,16 @@ Return ONLY the summary, nothing else."#,
         log::info!("tool_get_faces_in_photo: file_path='{}'", file_path);
 
         // Resolve content_hash from any library that has this rel_path.
-        // Walk libraries in their declared order and take the first hit.
-        let mut content_hash: Option<String> = None;
-        for lib in &self.libraries {
+        // Hold the FaceDao lock once across all libraries — resolve_content_hash
+        // is synchronous and there's no await in the loop body.
+        let content_hash: Option<String> = {
             let mut dao = self.face_dao.lock().expect("Unable to lock FaceDao");
-            if let Ok(Some(h)) = dao.resolve_content_hash(cx, lib.id, &file_path) {
-                content_hash = Some(h);
-                break;
-            }
-        }
+            self.libraries.iter().find_map(|lib| {
+                dao.resolve_content_hash(cx, lib.id, &file_path)
+                    .ok()
+                    .flatten()
+            })
+        };
         let Some(content_hash) = content_hash else {
             return "No content_hash found for that file path (the photo may not be indexed yet, \
                     or the path doesn't match any library)."
@@ -2215,9 +2216,11 @@ Return ONLY the summary, nothing else."#,
 
         let mut out = format!("Found {} face(s) in this photo:\n", faces.len());
         for f in &bound {
+            // Invariant: `bound` is filtered on `person_name.is_some()` above.
+            let name = f.person_name.as_deref().expect("bound face must have a name");
             out.push_str(&format!(
                 "- {} (confidence {:.2}, bbox x={:.2} y={:.2} w={:.2} h={:.2}, source: {})\n",
-                f.person_name.as_deref().unwrap_or("?"),
+                name,
                 f.confidence,
                 f.bbox_x,
                 f.bbox_y,
diff --git a/src/bin/populate_knowledge.rs b/src/bin/populate_knowledge.rs
index e72a27b..c2d1ade 100644
--- a/src/bin/populate_knowledge.rs
+++ b/src/bin/populate_knowledge.rs
@@ -201,8 +201,8 @@ async fn main() -> anyhow::Result<()> {
         location_dao,
         search_dao,
         tag_dao,
-        knowledge_dao,
         face_dao,
+        knowledge_dao,
         all_libs.clone(),
     );
 
diff --git a/src/faces.rs b/src/faces.rs
index d92b7ae..5b7a232 100644
--- a/src/faces.rs
+++ b/src/faces.rs
@@ -1437,7 +1437,6 @@ impl FaceDao for SqliteFaceDao {
     }
 
     fn has_any_faces(&mut self, ctx: &opentelemetry::Context) -> anyhow::Result<bool> {
-        use anyhow::Context;
         let mut conn = self.connection.lock().expect("face dao lock");
         trace_db_call(ctx, "query", "has_any_faces", |_span| {
             face_detections::table
diff --git a/src/state.rs b/src/state.rs
index 49d0b4c..adbf527 100644
--- a/src/state.rs
+++ b/src/state.rs
@@ -234,8 +234,8 @@ impl Default for AppState {
             location_dao.clone(),
             search_dao.clone(),
             tag_dao.clone(),
-            knowledge_dao,
             face_dao.clone(),
+            knowledge_dao,
             libraries_vec.clone(),
         );
 
@@ -376,8 +376,8 @@ impl AppState {
             location_dao.clone(),
             search_dao.clone(),
             tag_dao.clone(),
-            knowledge_dao,
             face_dao.clone(),
+            knowledge_dao,
             vec![test_lib],
         );