From 7aa707e7eefb53a9794c23a10de41e033ea585b8 Mon Sep 17 00:00:00 2001 From: vicrodh Date: Mon, 9 Mar 2026 13:16:58 -0600 Subject: [PATCH] feat: smart area resolution for scene discovery Resolve cities UP to subdivision level via MB area relations API: - Leyton -> England, Los Angeles -> California, Brixton -> England - Uses direction:"forward" to walk "part of" chain upward - Two-level inline lookup for districts/boroughs (avoids recursive async) - Bump per_genre_limit to 100 for broader coverage --- src-tauri/src/commands_v2.rs | 62 ++++++-- src-tauri/src/musicbrainz/client.rs | 222 ++++++++++++++++++++++++++++ src-tauri/src/musicbrainz/models.rs | 41 +++++ 3 files changed, 313 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/commands_v2.rs b/src-tauri/src/commands_v2.rs index 616b60c3f..c623ae940 100644 --- a/src-tauri/src/commands_v2.rs +++ b/src-tauri/src/commands_v2.rs @@ -8832,17 +8832,55 @@ pub async fn v2_discover_artists_by_location( .check_requirements(CommandRequirement::RequiresCoreBridgeAuth) .await?; - // Build display name for headers: "Country, City" or just area_name - let display_name = if let Some(ref c) = country { - format!("{}, {}", c, area_name) + // Step 0: Smart area resolution — resolve city → subdivision for broader results + // e.g., Leyton → England, Seattle → Washington + let (search_name, display_name) = if let Some(ref aid) = area_id { + // Try to resolve city to parent subdivision + match state.client.resolve_parent_subdivision(aid).await { + Ok(Some((subdivision_name, _subdivision_id))) => { + let display = if let Some(ref c) = country { + format!("{}, {}", c, subdivision_name) + } else { + subdivision_name.clone() + }; + log::info!( + "[V2] Area resolved: '{}' → subdivision '{}'", + area_name, + subdivision_name + ); + (subdivision_name, display) + } + Ok(None) => { + // No subdivision found, use area_name as-is + let display = if let Some(ref c) = country { + format!("{}, {}", c, area_name) + } else { + area_name.clone() + }; + (area_name.clone(), display) + } + Err(e) => { + log::warn!("[V2] Area resolution failed: {}, using '{}' directly", e, area_name); + let display = if let Some(ref c) = country { + format!("{}, {}", c, area_name) + } else { + area_name.clone() + }; + (area_name.clone(), display) + } + } } else { - area_name.clone() + let display = if let Some(ref c) = country { + format!("{}, {}", c, area_name) + } else { + area_name.clone() + }; + (area_name.clone(), display) }; log::info!( - "[V2] discover_artists_by_location: area={:?} search_name={} display={} genres={:?} offset={}", - area_id, - area_name, + "[V2] discover_artists_by_location: search='{}' display='{}' genres={:?} offset={}", + search_name, display_name, genres, offset @@ -8860,7 +8898,7 @@ pub async fn v2_discover_artists_by_location( }; // Build cache key from area + seeds — TEMPORARILY UNUSED - let _cache_key_area = area_id.as_deref().unwrap_or(&area_name); + let _cache_key_area = area_id.as_deref().unwrap_or(&search_name); let _cache_key = build_scene_cache_key(_cache_key_area, &source_seeds); // Step 1: Check scene cache — TEMPORARILY DISABLED for tuning @@ -8906,12 +8944,12 @@ pub async fn v2_discover_artists_by_location( // Deduplicate candidates across genre queries: mbid -> (name, score_sum, genre_hits, tags) let mut candidate_map: HashMap)> = HashMap::new(); - let per_genre_limit = 50; // Get up to 50 per genre query + let per_genre_limit = 100; // Get up to 100 per genre query for broader coverage for genre in &search_genres { let search_result = state .client - .search_artists_by_tag_and_area(genre, &area_name, per_genre_limit, 0) + .search_artists_by_tag_and_area(genre, &search_name, per_genre_limit, 0) .await; match search_result { @@ -8945,7 +8983,7 @@ pub async fn v2_discover_artists_by_location( .begin_area .as_ref() .map(|ba| { - ba.name.eq_ignore_ascii_case(&area_name) + ba.name.eq_ignore_ascii_case(&search_name) || area_id .as_deref() .map(|aid| ba.id == aid) @@ -8956,7 +8994,7 @@ pub async fn v2_discover_artists_by_location( let same_country = artist .area .as_ref() - .map(|a| a.name.eq_ignore_ascii_case(&area_name)) + .map(|a| a.name.eq_ignore_ascii_case(&search_name)) .unwrap_or(false); let score = compute_affinity_score( diff --git a/src-tauri/src/musicbrainz/client.rs b/src-tauri/src/musicbrainz/client.rs index 408ee91eb..f740060c3 100644 --- a/src-tauri/src/musicbrainz/client.rs +++ b/src-tauri/src/musicbrainz/client.rs @@ -699,6 +699,228 @@ impl MusicBrainzClient { .map_err(|e| format!("Failed to parse MusicBrainz response: {}", e)) } + /// Look up an area and its parent relationships. + /// + /// Used to resolve city → subdivision (e.g., Leyton → England). + /// Returns the area detail with `relations` that include "part of" links. + pub async fn get_area_with_relations( + &self, + area_id: &str, + ) -> Result { + if !self.is_enabled().await { + return Err("MusicBrainz integration is disabled".to_string()); + } + + self.rate_limiter.wait().await; + + let base_url = self.base_url().await; + let url = format!( + "{}/area/{}?inc=area-rels&fmt=json", + base_url, area_id + ); + + log::debug!("MusicBrainz area lookup with relations: {}", area_id); + + let response = self + .client + .get(&url) + .send() + .await + .map_err(|e| format!("MusicBrainz request failed: {}", e))?; + + if !response.status().is_success() { + let status = response.status(); + let text = response.text().await.unwrap_or_default(); + return Err(format!("MusicBrainz API error {}: {}", status, text)); + } + + response + .json::() + .await + .map_err(|e| format!("Failed to parse MusicBrainz area response: {}", e)) + } + + /// Resolve a city area to its parent subdivision (state/region). + /// + /// Uses MB area-rels "part of" with direction "forward" to go UP: + /// Los Angeles → "part of" (forward) → California (subdivision) + /// Leyton → "part of" (forward) → Waltham Forest → (forward) → England + /// + /// If the direct parent is too granular (district/municipality), walks + /// up one more level to find a proper subdivision. + pub async fn resolve_parent_subdivision( + &self, + area_id: &str, + ) -> Result, String> { + let detail = self.get_area_with_relations(area_id).await?; + + // Collect PARENT relations only (direction "forward" = "I am part of X") + let parents: Vec<_> = detail + .relations + .as_ref() + .map(|rels| { + rels.iter() + .filter(|rel| { + rel.relation_type == "part of" + && rel.direction.as_deref() == Some("forward") + }) + .filter_map(|rel| rel.area.as_ref()) + .collect() + }) + .unwrap_or_default(); + + if parents.is_empty() { + log::info!( + "No parent found for area '{}' ({}), type={:?}", + detail.name, + area_id, + detail.area_type + ); + return Ok(None); + } + + // Log all parents for debugging + for parent in &parents { + log::info!( + " Parent of '{}': '{}' ({}) type={:?}", + detail.name, + parent.name, + parent.id, + parent.area_type + ); + } + + // Priority 1: Find a subdivision parent + if let Some(subdiv) = parents.iter().find(|p| { + p.area_type + .as_deref() + .map(|t| t.eq_ignore_ascii_case("subdivision")) + .unwrap_or(false) + }) { + log::info!( + "Resolved '{}' → subdivision '{}' ({})", + detail.name, + subdiv.name, + subdiv.id + ); + return Ok(Some((subdiv.name.clone(), subdiv.id.clone()))); + } + + // Priority 2: Find a non-country, non-city parent (could be a region/state + // with a different type label) + if let Some(region) = parents.iter().find(|p| { + let t = p.area_type.as_deref().unwrap_or(""); + !t.is_empty() + && !t.eq_ignore_ascii_case("country") + && !t.eq_ignore_ascii_case("city") + && !t.eq_ignore_ascii_case("municipality") + }) { + log::info!( + "Resolved '{}' → {} '{}' ({})", + detail.name, + region.area_type.as_deref().unwrap_or("unknown"), + region.name, + region.id + ); + return Ok(Some((region.name.clone(), region.id.clone()))); + } + + // Priority 3: If the only parent is a country, check if the original area + // is already a subdivision — in that case, use it as-is + if let Some(country_parent) = parents.iter().find(|p| { + p.area_type + .as_deref() + .map(|t| t.eq_ignore_ascii_case("country")) + .unwrap_or(false) + }) { + let own_type = detail.area_type.as_deref().unwrap_or(""); + if own_type.eq_ignore_ascii_case("subdivision") { + // Area is already a subdivision (e.g., California, England) + log::info!( + "'{}' is already a subdivision (parent: {}), using as-is", + detail.name, + country_parent.name + ); + return Ok(None); // Don't resolve, the original name is fine + } + + // Parent is directly a country — the area might be a district/borough. + // Try walking up ONE more level from one of the non-country parents. + // This handles: Leyton → Waltham Forest (district) → ... → England + // But if only parent is country, just skip resolution. + log::info!( + "Only parent of '{}' is country '{}', no intermediate subdivision", + detail.name, + country_parent.name + ); + } + + // Priority 4: If parent is a district/municipality, walk up one more level + if let Some(district) = parents.first() { + let parent_type = district.area_type.as_deref().unwrap_or(""); + if parent_type.eq_ignore_ascii_case("district") + || parent_type.eq_ignore_ascii_case("municipality") + || parent_type.eq_ignore_ascii_case("city") + { + log::info!( + "Parent '{}' is a {}, walking up one more level...", + district.name, + parent_type + ); + // Walk up one level: resolve the parent's parent (non-recursive) + let grandparent = self.get_area_with_relations(&district.id).await?; + let gp_parents: Vec<_> = grandparent + .relations + .as_ref() + .map(|rels| { + rels.iter() + .filter(|r| { + r.relation_type == "part of" + && r.direction.as_deref() == Some("forward") + }) + .filter_map(|r| r.area.as_ref()) + .collect() + }) + .unwrap_or_default(); + + // Find subdivision among grandparents + if let Some(subdiv) = gp_parents.iter().find(|p| { + p.area_type + .as_deref() + .map(|t| t.eq_ignore_ascii_case("subdivision")) + .unwrap_or(false) + }) { + log::info!( + "Resolved '{}' → '{}' → subdivision '{}' ({})", + detail.name, + district.name, + subdiv.name, + subdiv.id + ); + return Ok(Some((subdiv.name.clone(), subdiv.id.clone()))); + } + + // If no subdivision, use the grandparent district's non-country parent + if let Some(region) = gp_parents.iter().find(|p| { + let t = p.area_type.as_deref().unwrap_or(""); + !t.is_empty() && !t.eq_ignore_ascii_case("country") + }) { + log::info!( + "Resolved '{}' → '{}' → {} '{}' ({})", + detail.name, + district.name, + region.area_type.as_deref().unwrap_or("unknown"), + region.name, + region.id + ); + return Ok(Some((region.name.clone(), region.id.clone()))); + } + } + } + + Ok(None) + } + /// Escape special characters in Lucene queries fn escape_query(s: &str) -> String { s.replace('\\', "\\\\") diff --git a/src-tauri/src/musicbrainz/models.rs b/src-tauri/src/musicbrainz/models.rs index 598287eaf..f348c4723 100644 --- a/src-tauri/src/musicbrainz/models.rs +++ b/src-tauri/src/musicbrainz/models.rs @@ -389,6 +389,47 @@ pub struct AreaResult { pub life_span: Option, } +/// Area detail response (from /area/{id}?inc=area-rels) +#[derive(Debug, Deserialize)] +pub struct AreaDetailResponse { + pub id: String, + pub name: String, + #[serde(rename = "sort-name")] + pub sort_name: Option, + #[serde(rename = "type")] + pub area_type: Option, + #[serde(rename = "iso-3166-1-codes", default)] + pub iso_codes: Option>, + #[serde(rename = "iso-3166-2-codes", default)] + pub iso_3166_2_codes: Option>, + #[serde(default)] + pub relations: Option>, +} + +/// Area relationship entry +#[derive(Debug, Deserialize)] +pub struct AreaRelation { + #[serde(rename = "type")] + pub relation_type: String, + pub direction: Option, + pub area: Option, +} + +/// Target area in an area relationship +#[derive(Debug, Deserialize)] +pub struct AreaRelTarget { + pub id: String, + pub name: String, + #[serde(rename = "sort-name")] + pub sort_name: Option, + #[serde(rename = "type")] + pub area_type: Option, + #[serde(rename = "iso-3166-1-codes", default)] + pub iso_codes: Option>, + #[serde(rename = "iso-3166-2-codes", default)] + pub iso_3166_2_codes: Option>, +} + // ============ Resolved Types (Output) ============ /// Resolved track with MusicBrainz data