diff --git a/backend/src/handlers/service_pool_inspection_search_tests.rs b/backend/src/handlers/service_pool_inspection_search_tests.rs new file mode 100644 index 000000000..30a4da8da --- /dev/null +++ b/backend/src/handlers/service_pool_inspection_search_tests.rs @@ -0,0 +1,400 @@ +use super::*; + +#[tokio::test] +async fn pool_inspection_name_search_precedes_paging_and_preserves_scope() { + let mut fixture = fixture("pool_name_search", StatusCode::OK, "priority", false).await; + let db = &fixture.state.db; + let services: Vec = db + .collection::("user_services") + .find(doc! {}) + .sort(doc! {"_id":1}) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + let first_id = services[0].get_str("_id").unwrap(); + let last = &services[1]; + let last_id = last.get_str("_id").unwrap(); + let original_catalog_id = last.get_str("catalog_service_id").unwrap().to_owned(); + let label = "Preferred member [West]"; + db.collection::("user_endpoints") + .update_one( + doc! {"_id":last.get_str("endpoint_id").unwrap()}, + doc! {"$set":{"label":label}}, + ) + .await + .unwrap(); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + let page = inspect(&fixture, json!({"check_operation":false,"limit":1}), false).await; + assert_eq!(page.candidates[0].user_service_id, first_id); + for search in ["preferred MEMBER [west]", last.get_str("slug").unwrap()] { + let found = inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false, + ) + .await; + assert_eq!(found.candidates.len(), 1); + assert_eq!(found.candidates[0].user_service_id, last_id); + assert_eq!(found.candidates[0].name, label); + assert_ne!(found.candidates[0].group_name, "Custom connections"); + assert!(found.candidates[0].group_slug.is_some()); + assert!(!found.has_more); + } + // Only the later inventory row references this catalog. Matching its literal + // original name or slug must happen before limit/skip, not on a loaded page. + let catalog_id = Uuid::new_v4().to_string(); + let catalog_name = "Original [West].* (Pool)+"; + let catalog_slug = "original-west-pool"; + let mut catalog = db + .collection::("downstream_services") + .find_one(doc! {"_id":last.get_str("catalog_service_id").unwrap()}) + .await + .unwrap() + .unwrap(); + catalog.insert("_id", &catalog_id); + catalog.insert("name", catalog_name); + catalog.insert("slug", catalog_slug); + db.collection::("downstream_services") + .insert_one(catalog) + .await + .unwrap(); + db.collection::("user_services") + .update_one( + doc! {"_id":last_id}, + doc! {"$set":{"catalog_service_id":&catalog_id}}, + ) + .await + .unwrap(); + for search in ["original [west].* (pool)+", "ORIGINAL-WEST-POOL", ".*"] { + let found = inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false, + ) + .await; + assert_eq!(found.candidates.len(), 1, "{search}"); + assert_eq!(found.candidates[0].user_service_id, last_id, "{search}"); + assert_eq!(found.candidates[0].group_name, catalog_name); + assert_eq!( + found.candidates[0].group_slug.as_deref(), + Some(catalog_slug) + ); + assert_eq!( + found.candidates[0].catalog_service_id.as_deref(), + Some(catalog_id.as_str()) + ); + assert!(!found.has_more); + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1,"after":"1"}), + false + ) + .await + .candidates + .is_empty() + ); + } + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":"[not-present]"}), + false + ) + .await + .candidates + .is_empty() + ); + // Matching foreign-owned instances must never enter the candidate result. + let mut foreign = last.clone(); + foreign.insert("_id", Uuid::new_v4().to_string()); + foreign.insert("user_id", Uuid::new_v4().to_string()); + foreign.insert("catalog_service_id", &catalog_id); + db.collection::("user_services") + .insert_one(foreign) + .await + .unwrap(); + assert_eq!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .len(), + 1 + ); + for search in [catalog_name, catalog_slug] { + let found = inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false, + ) + .await; + assert_eq!(found.candidates.len(), 1); + assert_eq!(found.candidates[0].user_service_id, last_id); + assert!( + !found.has_more, + "foreign owned catalog match must not affect paging" + ); + } + fixture.auth.allow_all_services = false; + fixture.auth.allowed_service_ids = vec![first_id.to_owned()]; + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .is_empty() + ); + for search in [catalog_name, catalog_slug] { + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false + ) + .await + .candidates + .is_empty() + ); + } + let selected = inspect(&fixture, json!({"check_operation":false,"search":"no match","selected_only":true,"peer_ids":format!("{first_id},{last_id}"),"limit":1,"after":"999"}), false).await; + assert_eq!(selected.candidates.len(), 1); + assert_eq!(selected.candidates[0].user_service_id, first_id); + fixture.auth.allow_all_services = true; + // Platform resolution also displays the endpoint label, with no credential materialization. + db.collection::("downstream_services").update_one( + doc! {"_id":&original_catalog_id}, + doc! {"$set":{"auth_method":"bearer","service_category":"internal","requires_user_credential":false,"visibility":"public","credential_encrypted":mongodb::bson::Binary{subtype:mongodb::bson::spec::BinarySubtype::Generic,bytes:vec![1]}}}, + ).await.unwrap(); + db.collection::("user_services") + .update_one( + doc! {"_id":last_id}, + doc! {"$set":{"catalog_service_id":&original_catalog_id,"credential_binding":"platform","auth_method":"bearer"}}, + ) + .await + .unwrap(); + let platform = inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false, + ) + .await; + assert_eq!(platform.candidates.len(), 1); + assert_eq!(platform.candidates[0].name, label); + assert_eq!(platform.candidates[0].credential_binding, "platform"); + assert!(platform.candidates[0].eligible); + // Matching-name pages retain stable pagination, without consuming unrelated rows. + db.collection::("user_endpoints") + .update_one( + doc! {"_id":services[0].get_str("endpoint_id").unwrap()}, + doc! {"$set":{"label":"Another member [West]"}}, + ) + .await + .unwrap(); + let page = inspect( + &fixture, + json!({"check_operation":false,"search":"member [West]","limit":1}), + false, + ) + .await; + assert_eq!(page.candidates.len(), 1); + assert!(page.has_more); + let next = inspect(&fixture, json!({"check_operation":false,"search":"member [West]","limit":1,"after":page.next_cursor}), false).await; + assert_eq!(next.candidates.len(), 1); + assert!(!next.has_more); + assert_ne!( + page.candidates[0].user_service_id, + next.candidates[0].user_service_id + ); + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + assert!(fixture.first.requests.lock().await.is_empty()); + assert!(fixture.second.requests.lock().await.is_empty()); + db.drop().await.unwrap(); +} + +#[tokio::test] +async fn pool_inspection_renamed_key_labels_match_services_without_exposing_foreign_or_platform_keys() + { + let mut fixture = fixture("pool_renamed_key", StatusCode::OK, "priority", false).await; + bind_first_credential(&fixture).await; + let db = &fixture.state.db; + let service = db + .collection::("user_services") + .find_one(doc! {"slug":"review-first"}) + .await + .unwrap() + .unwrap(); + let id = service.get_str("_id").unwrap(); + let key_id = service.get_str("api_key_id").unwrap(); + let owner = service.get_str("user_id").unwrap(); + let endpoint_id = service.get_str("endpoint_id").unwrap(); + db.collection::("user_endpoints") + .update_one( + doc! {"_id":endpoint_id}, + doc! {"$set":{"label":"Previous endpoint label"}}, + ) + .await + .unwrap(); + let label = "Team [renamed].*"; + crate::services::user_api_key_service::update_api_key( + db, + &fixture.state.encryption_keys, + owner, + key_id, + Some(label), + None, + ) + .await + .unwrap(); + let view = crate::services::unified_key_service::get_key( + db, + &fixture.state.encryption_keys, + owner, + id, + ) + .await + .unwrap(); + assert_eq!(view.label, label); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + let key_before = db + .collection::("user_api_keys") + .find_one(doc! {"_id":key_id}) + .await + .unwrap() + .unwrap(); + for status in ["active", "failed"] { + db.collection::("user_api_keys") + .update_one(doc! {"_id":key_id}, doc! {"$set":{"status":status}}) + .await + .unwrap(); + for search in ["TEAM [renamed].*", "[renamed]", ".*"] { + let found = inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false, + ) + .await; + assert_eq!(found.candidates.len(), 1); + assert_eq!(found.candidates[0].user_service_id, id); + assert_eq!(found.candidates[0].name, label); + assert!(!found.has_more); + } + for (query, health) in [ + ( + json!({"check_operation":false,"selected_only":true,"peer_ids":id}), + false, + ), + (json!({"check_operation":false}), true), + ] { + let found = inspect(&fixture, query, health).await; + assert_eq!( + found + .candidates + .iter() + .find(|r| r.user_service_id == id) + .unwrap() + .name, + label + ); + } + } + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":"Previous endpoint label"}), + false + ) + .await + .candidates + .is_empty() + ); + fixture.auth.allow_all_services = false; + fixture.auth.allowed_service_ids = vec![fixture.second_member_id.clone()]; + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .is_empty() + ); + fixture.auth.allow_all_services = true; + // Platform bindings use the endpoint label even if a personal key is retained. + db.collection::("user_services") + .update_one( + doc! {"_id":id}, + doc! {"$set":{"credential_binding":"platform"}}, + ) + .await + .unwrap(); + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .is_empty() + ); + let platform = inspect( + &fixture, + json!({"check_operation":false,"search":"Previous endpoint label"}), + false, + ) + .await; + assert_eq!(platform.candidates[0].name, "Previous endpoint label"); + db.collection::("user_services") + .update_one(doc! {"_id":id}, doc! {"$unset":{"credential_binding":""}}) + .await + .unwrap(); + // An inconsistent foreign key reference cannot disclose its label in display or search. + db.collection::("user_api_keys") + .update_one( + doc! {"_id":key_id}, + doc! {"$set":{"user_id":Uuid::new_v4().to_string()}}, + ) + .await + .unwrap(); + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .is_empty() + ); + let foreign = inspect( + &fixture, + json!({"check_operation":false,"search":"Previous endpoint label"}), + false, + ) + .await; + assert_eq!(foreign.candidates[0].name, "Previous endpoint label"); + let key_after = db + .collection::("user_api_keys") + .find_one(doc! {"_id":key_id}) + .await + .unwrap() + .unwrap(); + assert_eq!( + key_before.get("last_used_at"), + key_after.get("last_used_at") + ); + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + assert!(fixture.first.requests.lock().await.is_empty()); + assert!(fixture.second.requests.lock().await.is_empty()); + db.drop().await.unwrap(); +} diff --git a/backend/src/handlers/service_pool_inspection_tests.rs b/backend/src/handlers/service_pool_inspection_tests.rs index d3828d757..ba4091054 100644 --- a/backend/src/handlers/service_pool_inspection_tests.rs +++ b/backend/src/handlers/service_pool_inspection_tests.rs @@ -632,137 +632,5 @@ fn pool_credential_unavailability_is_narrowly_classified() { ))); } -#[tokio::test] -async fn pool_inspection_name_search_precedes_paging_and_preserves_scope() { - let mut fixture = fixture("pool_name_search", StatusCode::OK, "priority", false).await; - let db = &fixture.state.db; - let services: Vec = db - .collection::("user_services") - .find(doc! {}) - .sort(doc! {"_id":1}) - .await - .unwrap() - .try_collect() - .await - .unwrap(); - let first_id = services[0].get_str("_id").unwrap(); - let last = &services[1]; - let last_id = last.get_str("_id").unwrap(); - let label = "Preferred member [West]"; - db.collection::("user_endpoints") - .update_one( - doc! {"_id":last.get_str("endpoint_id").unwrap()}, - doc! {"$set":{"label":label}}, - ) - .await - .unwrap(); - let decrypts = fixture.state.encryption_keys.decrypt_stats(); - let page = inspect(&fixture, json!({"check_operation":false,"limit":1}), false).await; - assert_eq!(page.candidates[0].user_service_id, first_id); - for search in ["preferred MEMBER [west]", last.get_str("slug").unwrap()] { - let found = inspect( - &fixture, - json!({"check_operation":false,"search":search,"limit":1}), - false, - ) - .await; - assert_eq!(found.candidates.len(), 1); - assert_eq!(found.candidates[0].user_service_id, last_id); - assert_eq!(found.candidates[0].name, label); - assert!(!found.has_more); - } - assert!( - inspect( - &fixture, - json!({"check_operation":false,"search":".*"}), - false - ) - .await - .candidates - .is_empty() - ); - // Matching foreign-owned instances must never enter the candidate result. - let mut foreign = last.clone(); - foreign.insert("_id", Uuid::new_v4().to_string()); - foreign.insert("user_id", Uuid::new_v4().to_string()); - db.collection::("user_services") - .insert_one(foreign) - .await - .unwrap(); - assert_eq!( - inspect( - &fixture, - json!({"check_operation":false,"search":label}), - false - ) - .await - .candidates - .len(), - 1 - ); - fixture.auth.allow_all_services = false; - fixture.auth.allowed_service_ids = vec![first_id.to_owned()]; - assert!( - inspect( - &fixture, - json!({"check_operation":false,"search":label}), - false - ) - .await - .candidates - .is_empty() - ); - let selected = inspect(&fixture, json!({"check_operation":false,"search":"no match","selected_only":true,"peer_ids":format!("{first_id},{last_id}"),"limit":1,"after":"999"}), false).await; - assert_eq!(selected.candidates.len(), 1); - assert_eq!(selected.candidates[0].user_service_id, first_id); - fixture.auth.allow_all_services = true; - // Platform resolution also displays the endpoint label, with no credential materialization. - db.collection::("downstream_services").update_one( - doc! {"_id":last.get_str("catalog_service_id").unwrap()}, - doc! {"$set":{"auth_method":"bearer","service_category":"internal","requires_user_credential":false,"visibility":"public","credential_encrypted":mongodb::bson::Binary{subtype:mongodb::bson::spec::BinarySubtype::Generic,bytes:vec![1]}}}, - ).await.unwrap(); - db.collection::("user_services") - .update_one( - doc! {"_id":last_id}, - doc! {"$set":{"credential_binding":"platform","auth_method":"bearer"}}, - ) - .await - .unwrap(); - let platform = inspect( - &fixture, - json!({"check_operation":false,"search":label}), - false, - ) - .await; - assert_eq!(platform.candidates.len(), 1); - assert_eq!(platform.candidates[0].name, label); - assert_eq!(platform.candidates[0].credential_binding, "platform"); - assert!(platform.candidates[0].eligible); - // Matching-name pages retain stable pagination, without consuming unrelated rows. - db.collection::("user_endpoints") - .update_one( - doc! {"_id":services[0].get_str("endpoint_id").unwrap()}, - doc! {"$set":{"label":"Another member [West]"}}, - ) - .await - .unwrap(); - let page = inspect( - &fixture, - json!({"check_operation":false,"search":"member [West]","limit":1}), - false, - ) - .await; - assert_eq!(page.candidates.len(), 1); - assert!(page.has_more); - let next = inspect(&fixture, json!({"check_operation":false,"search":"member [West]","limit":1,"after":page.next_cursor}), false).await; - assert_eq!(next.candidates.len(), 1); - assert!(!next.has_more); - assert_ne!( - page.candidates[0].user_service_id, - next.candidates[0].user_service_id - ); - assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); - assert!(fixture.first.requests.lock().await.is_empty()); - assert!(fixture.second.requests.lock().await.is_empty()); - db.drop().await.unwrap(); -} +#[path = "service_pool_inspection_search_tests.rs"] +mod search; diff --git a/backend/src/handlers/service_pools_handler.rs b/backend/src/handlers/service_pools_handler.rs index e8992fdc7..bfbe07c87 100644 --- a/backend/src/handlers/service_pools_handler.rs +++ b/backend/src/handlers/service_pools_handler.rs @@ -599,6 +599,10 @@ pub struct PoolCandidateResponse { pub credential_binding: String, pub protocol: Option, pub catalog_service_id: Option, + /// Authoritative original catalog grouping metadata. Custom connections + /// use the stable "Custom connections" group and a null catalog ID. + pub group_name: String, + pub group_slug: Option, pub requires_compatibility_declaration: bool, pub cooldown_until: Option, pub consecutive_failures: i64, @@ -722,6 +726,8 @@ async fn inspect_pool_candidates( credential_binding: row.credential_binding, protocol: row.protocol, catalog_service_id: row.catalog_service_id, + group_name: row.group_name, + group_slug: row.group_slug, requires_compatibility_declaration: row.requires_compatibility_declaration, cooldown_until: row.cooldown_until.map(|t| t.to_rfc3339()), consecutive_failures: row.consecutive_failures, diff --git a/backend/src/services/service_pool_inspection.rs b/backend/src/services/service_pool_inspection.rs index b53bf3848..881554c4b 100644 --- a/backend/src/services/service_pool_inspection.rs +++ b/backend/src/services/service_pool_inspection.rs @@ -4,14 +4,31 @@ use crate::{ crypto::aes::EncryptionKeys, errors::{AppError, AppResult}, models::{ + downstream_service::COLLECTION_NAME as DOWNSTREAM_SERVICES, service_pool::{PoolMemberContract, PoolStrategy, ServicePool}, user_service::UserService, }, }; use futures::TryStreamExt; use mongodb::bson::doc; +use serde::Deserialize; use std::collections::HashSet; +#[derive(Debug, Deserialize)] +struct CatalogDisplayMetadata { + #[serde(rename = "_id")] + id: String, + name: String, + slug: String, +} + +#[derive(Debug, Deserialize)] +struct ConnectionDisplayMetadata { + #[serde(rename = "_id")] + id: String, + label: String, +} + pub struct CandidateInspection { pub user_service_id: String, pub name: String, @@ -22,6 +39,8 @@ pub struct CandidateInspection { pub credential_binding: String, pub protocol: Option, pub catalog_service_id: Option, + pub group_name: String, + pub group_slug: Option, pub requires_compatibility_declaration: bool, pub cooldown_until: Option>, pub consecutive_failures: i64, @@ -122,7 +141,7 @@ pub async fn inspect( .search .filter(|search| !query.members_only && !query.selected_only && !search.is_empty()); let mut services: Vec = if let Some(search) = search { - search_services(db, filter, search, offset, limit + 1).await? + search_services(db, filter, owner, search, offset, limit + 1).await? } else { crate::services::service_history::collection(db, "user_services") .find(filter) @@ -136,6 +155,65 @@ pub async fn inspect( let has_more = services.len() > limit; services.truncate(limit); let next_cursor = has_more.then(|| (offset + limit as u64).to_string()); + + // Resolve only the bounded page's safe display metadata. This keeps a + // failed credential from hiding its connection label and lets the picker + // group by the authoritative catalog identity without materializing any + // credential or broadening the inventory ACL. + let catalog_ids: Vec = services + .iter() + .filter_map(|service| service.catalog_service_id.clone()) + .collect(); + let catalog_metadata: std::collections::HashMap = + if catalog_ids.is_empty() { + std::collections::HashMap::new() + } else { + db.collection::(DOWNSTREAM_SERVICES) + .find(doc! {"_id": {"$in": &catalog_ids}}) + .projection(doc! {"_id": 1, "name": 1, "slug": 1}) + .await? + .try_collect::>() + .await? + .into_iter() + .map(|service| (service.id, (service.name, service.slug))) + .collect() + }; + let endpoint_ids: Vec = services + .iter() + .map(|service| service.endpoint_id.clone()) + .filter(|id| !id.is_empty()) + .collect(); + let endpoint_labels: std::collections::HashMap = if endpoint_ids.is_empty() { + std::collections::HashMap::new() + } else { + db.collection::("user_endpoints") + .find(doc! {"_id": {"$in": &endpoint_ids}, "user_id": owner}) + .projection(doc! {"_id": 1, "label": 1}) + .await? + .try_collect::>() + .await? + .into_iter() + .map(|endpoint| (endpoint.id, endpoint.label)) + .collect() + }; + let key_ids: Vec = services + .iter() + .filter(|service| super::platform_key_service::binding(service) != "platform") + .filter_map(|service| service.api_key_id.clone()) + .collect(); + let key_labels: std::collections::HashMap = if key_ids.is_empty() { + std::collections::HashMap::new() + } else { + db.collection::("user_api_keys") + .find(doc! {"_id": {"$in": &key_ids}, "user_id": owner}) + .projection(doc! {"_id": 1, "label": 1}) + .await? + .try_collect::>() + .await? + .into_iter() + .map(|key| (key.id, key.label)) + .collect() + }; let selected = if let Some(peers) = query.peer_ids.filter(|_| !query.members_only) { let members: Vec<_> = peers .iter() @@ -169,7 +247,15 @@ pub async fn inspect( }); let mut row = CandidateInspection { user_service_id: service.id.clone(), - name: service.slug.clone(), + name: service + .api_key_id + .as_ref() + .filter(|_| super::platform_key_service::binding(&service) != "platform") + .and_then(|id| key_labels.get(id)) + .or_else(|| endpoint_labels.get(&service.endpoint_id)) + .filter(|label| !label.is_empty()) + .cloned() + .unwrap_or_else(|| service.slug.clone()), slug: service.slug.clone(), is_active: service.is_active, eligible: true, @@ -180,6 +266,17 @@ pub async fn inspect( .unwrap_or_else(|| "user".into()), protocol: None, catalog_service_id: service.catalog_service_id.clone(), + group_name: match service.catalog_service_id.as_ref() { + Some(id) => catalog_metadata + .get(id) + .map(|metadata| metadata.0.clone()) + .unwrap_or_else(|| "Unavailable catalog service".into()), + None => "Custom connections".into(), + }, + group_slug: service + .catalog_service_id + .as_ref() + .and_then(|id| catalog_metadata.get(id).map(|metadata| metadata.1.clone())), requires_compatibility_declaration: service.catalog_service_id.is_none(), cooldown_until: None, consecutive_failures: 0, @@ -200,7 +297,6 @@ pub async fn inspect( .await; match resolution { Ok(Some(resolution)) => { - row.name = resolution.target.service.name.clone(); row.credential_binding = if resolution.master_credential { "platform" } else if resolution.target.auth_method == "none" { @@ -383,6 +479,8 @@ pub async fn inspect( credential_binding: "unavailable".into(), protocol: None, catalog_service_id: None, + group_name: "Custom connections".into(), + group_slug: None, requires_compatibility_declaration: false, cooldown_until: None, consecutive_failures: 0, @@ -438,11 +536,12 @@ pub async fn inspect( }) } -/// Search before pagination, projecting only the referenced endpoint label. -/// Both personal and platform resolution use this label as the displayed name. +/// Search connection and original catalog labels before pagination. +/// Lookups project display metadata only, after owner and caller scope filtering. async fn search_services( db: &mongodb::Database, filter: mongodb::bson::Document, + owner: &str, search: &str, offset: u64, limit: usize, @@ -455,12 +554,36 @@ async fn search_services( doc! {"$sort":{"_id":1}}, doc! {"$lookup":{ "from":"user_endpoints","localField":"endpoint_id","foreignField":"_id", - "pipeline":[{"$project":{"_id":0,"label":1}}],"as":"search_endpoint", + "pipeline":[ + {"$match":{"user_id":owner}}, + {"$project":{"_id":0,"label":1}}, + ],"as":"search_endpoint", + }}, + doc! {"$lookup":{ + "from":DOWNSTREAM_SERVICES,"localField":"catalog_service_id","foreignField":"_id", + "pipeline":[{"$project":{"_id":0,"name":1,"slug":1}}],"as":"search_catalog", + }}, + doc! {"$lookup":{ + "from":"user_api_keys","localField":"api_key_id","foreignField":"_id", + "let":{"binding":"$credential_binding"}, + "pipeline":[ + {"$match":{"user_id":owner,"$expr":{"$ne":[{"$ifNull":["$$binding","user"]},"platform"]}}}, + {"$project":{"_id":0,"label":1}}, + ],"as":"search_key", }}, - doc! {"$match":{"$or":[{"slug":&pattern},{"search_endpoint.label":pattern}]}}, + doc! {"$set":{"search_label":{"$ifNull":[ + {"$arrayElemAt":["$search_key.label",0]}, + {"$arrayElemAt":["$search_endpoint.label",0]}, + ]}}}, + doc! {"$match":{"$or":[ + {"slug":&pattern}, + {"search_label":&pattern}, + {"search_catalog.name":&pattern}, + {"search_catalog.slug":&pattern}, + ]}}, doc! {"$skip":offset as i64}, doc! {"$limit":limit as i64}, - doc! {"$unset":"search_endpoint"}, + doc! {"$unset":["search_endpoint","search_catalog","search_key","search_label"]}, ]) .max_time(std::time::Duration::from_secs(5)) .await? diff --git a/backend/src/services/service_pool_service.rs b/backend/src/services/service_pool_service.rs index 8095757fc..a64d638f6 100644 --- a/backend/src/services/service_pool_service.rs +++ b/backend/src/services/service_pool_service.rs @@ -77,12 +77,12 @@ pub struct UpdatePoolInput { } fn validate_text_fields(name: &str, description: Option<&str>) -> AppResult<()> { - if name.trim().is_empty() || name.len() > MAX_NAME_LEN { + if name.trim().is_empty() || name.chars().count() > MAX_NAME_LEN { return Err(AppError::ValidationError(format!( "Pool name must be 1-{MAX_NAME_LEN} characters" ))); } - if description.is_some_and(|d| d.len() > MAX_DESCRIPTION_LEN) { + if description.is_some_and(|d| d.chars().count() > MAX_DESCRIPTION_LEN) { return Err(AppError::ValidationError(format!( "description must not exceed {MAX_DESCRIPTION_LEN} characters" ))); @@ -121,7 +121,7 @@ fn normalize_members(members: Vec) -> AppResult MAX_MODEL_LEN { + } else if trimmed.chars().count() > MAX_MODEL_LEN { return Err(AppError::ServicePoolMemberInvalid(format!( "member model must not exceed {MAX_MODEL_LEN} characters" ))); @@ -1179,6 +1179,10 @@ pub(crate) fn is_duplicate_key(err: &mongodb::error::Error) -> bool { ) } +#[cfg(test)] +#[path = "service_pool_validation_tests.rs"] +mod validation_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/backend/src/services/service_pool_validation_tests.rs b/backend/src/services/service_pool_validation_tests.rs new file mode 100644 index 000000000..31b9d27d5 --- /dev/null +++ b/backend/src/services/service_pool_validation_tests.rs @@ -0,0 +1,34 @@ +use super::*; + +#[test] +fn pool_text_limits_count_unicode_characters() { + for character in ["a", "服", "🪐"] { + assert!( + validate_text_fields(&character.repeat(128), Some(&character.repeat(1024))).is_ok() + ); + assert!(validate_text_fields(&character.repeat(129), None).is_err()); + assert!(validate_text_fields("Pool", Some(&character.repeat(1025))).is_err()); + let member = ServicePoolMember { + user_service_id: "member".into(), + weight: 1, + enabled: true, + priority: 0, + model: Some(format!(" {} ", character.repeat(256))), + same_api_compatible: false, + health_reset_generation: 0, + }; + let normalized = normalize_members(vec![member.clone()]).unwrap(); + assert_eq!( + normalized[0].model.as_deref(), + Some(character.repeat(256).as_str()) + ); + assert!( + normalize_members(vec![ServicePoolMember { + model: Some(character.repeat(257)), + ..member + }]) + .is_err() + ); + } + assert!(validate_text_fields(" ", None).is_err()); +} diff --git a/docs/API.md b/docs/API.md index be9ee30c5..92c01b397 100644 --- a/docs/API.md +++ b/docs/API.md @@ -3771,7 +3771,7 @@ Candidate and health inspection accept these query parameters: | `strategy`, `member_contract` | Draft routing strategy and contract for candidate inspection. Health uses the saved configuration. | | `peer_ids`, `declared_peer_ids` | Comma-separated draft connection UUIDs and Same API compatibility declarations, capped at 50 IDs each. | | `selected_only` | With `true`, candidate inspection returns the selected `peer_ids` independently of search and pagination. | -| `search`, `limit`, `after` | Case-insensitive name or slug search, applied before candidate pagination. `limit` defaults to 100 and is clamped to 1–100. | +| `search`, `limit`, `after` | Literal, case-insensitive search over connection and original catalog service names and slugs, applied before candidate pagination. `limit` defaults to 100 and is clamped to 1–100. | | `org_id` | Organization owner for new-pool candidate inspection. Existing-pool routes resolve the owner from the pool. | Responses contain `operation_checked`, `method`, `path`, `candidates`, @@ -3781,6 +3781,18 @@ its connection ID, `name`, `slug`, eligibility and reason, credential binding, protocol, compatibility requirements, and cooldown metadata. Inventory results do not establish that a particular operation can execute. Inspection is read-only and never decrypts credentials or sends a request to a provider. +The connection `name` and name search use the same label as the Services page: +the owned stored key's label, falling back to the owned endpoint label and then +the connection slug. Explicit platform bindings use the endpoint label even +when a personal key is retained. Connection-label lookups project only IDs and labels. + +Candidates also carry `catalog_service_id`, `group_name`, and `group_slug` for +grouped connection selectors. Catalog-backed connections use the original +service's name and slug; group them by `catalog_service_id`, since different +services can share a display name. Custom connections have a null catalog ID, +`group_name: "Custom connections"`, and a null `group_slug`. Group metadata +describes only the returned connections; pagination still counts connections, +and one service's connections may span multiple pages. A connection with an inactive stored credential or missing credential material returns `eligible: false` with `reason: "credential_unavailable"`; other diff --git a/docs/SERVICE_POOLS.md b/docs/SERVICE_POOLS.md index aa6dcb7c7..72f9cbb6c 100644 --- a/docs/SERVICE_POOLS.md +++ b/docs/SERVICE_POOLS.md @@ -17,7 +17,18 @@ billing classification. Pools belong to a person or organization. Priority pools have a separate `tier_balance` of `round_robin` or `weighted`. Backup tiers advance only when visited, so intermittent primary failures still distribute work across backups. Members have weight 1–1000 and may be disabled -without deleting their connection. +without deleting their connection. The dashboard can reorder connections within +each priority tier without changing their priorities or weights. Each tier uses +a separate counter, and saving configuration restarts these tier cycles. + +Round-robin and weighted routing use the saved member order for a repeating +cycle. Round-robin gives each eligible member one turn. Weighted routing gives +each member as many consecutive turns as its weight: A with weight 2 followed +by B with weight 1 produces A → A → B, then repeats. Reordering those members +produces B → A → A while preserving their 1/3 and 2/3 shares. The next request +continues from the pool's current counter; saving does not restart the cycle. +Disabled or unavailable members are omitted at execution time. Ordered retry +after a failed attempt requires the `priority` strategy. A priority pool uses one of two request contracts: @@ -268,8 +279,17 @@ primary credential does not block a healthy backup. Inspection checks stored state; it does not test credentials against the provider. Use the dashboard's searchable multi-select dropdown to choose connections. -Search matches connection names and slugs across the inventory. -It stays open while selecting; select a checked connection again to remove it. +Connections are grouped under their original catalog service, with individual +accounts and keys visible beneath each heading. Custom connections have their +own group. Search matches connection and original service names and slugs +across the inventory. Connection names match the Services page, including renamed +keys. Platform connections use their endpoint label rather than a retained +personal key's label. +It stays open while selecting and retains loaded pages and scroll position; +select a checked connection again to remove it. +While compatibility is being checked, new selections and pagination wait for the +result. You can still remove selected connections. An in-flight next page finishes +before the loaded pages refresh against the latest selection. The selected connections appear in the form with priority and model settings. Unavailable connections show a repair reason and cannot be added. The dashboard requires at least one selected connection when creating a pool. @@ -299,6 +319,18 @@ instead of silently overwriting concurrent changes. Omitted fields preserve the current value. Null clears description/model/failover; a members array replaces membership while preserving omitted fields for retained IDs. The dashboard sends one PUT, including `expected_revision`, for all edited settings and members. +After a conflict, **Reload latest** replaces the draft with the current saved +configuration and revision. The pool list also refreshes, so closing and reopening +the editor loads the latest settings. + +Switching routing modes keeps the draft's priorities, models, balancing and retry +settings. Saving sends only the fields used by the selected mode. Invalid hidden +weights are replaced with 1 for an unweighted mode; visible weights must be whole +numbers from 1 to 1000. Cycle previews show enabled connections and the consecutive +turns assigned by each weight. + +Name, description and model limits are 128, 1024 and 256 Unicode characters, +respectively, using the same Unicode scalar count in the form and API. `set-strategy` can also change tier balancing and uses a revision-checked PUT. `add-member` updates existing members while preserving omitted fields; its flags cover priority, weight, enabled state, model, `--clear-model` and @@ -329,9 +361,13 @@ list saved members without claiming that cooldown has been checked. The dashboard provides one Create Pool action. New connections get increasing priorities so the common primary/backup setup works without editing priority numbers. Same API and AI chat choices explain request behavior; switching back to -Same API clears hidden model mappings. Retry limits, same-priority balancing, -description and enabled state live under Advanced settings. Search and pagination -never discard draft members or their compatibility confirmations. Editing saves +Same API clears hidden model mappings. Same-priority balancing appears beside +the connection controls. Weighted and round-robin connections have explicit cycle +positions and move controls; weighted connections also show their configured +share among enabled connections (within the same priority tier for fallback). +Retry limits, description and pool enabled state live under Advanced settings. +Search and pagination never discard draft members or their compatibility +confirmations. Editing saves one revision-checked update; stale revisions remain visible as conflicts. Responses include `x-nyxid-pool-member` and `x-nyxid-pool-attempts`. Exhaustion diff --git a/docs/plans/service-pool-relook-validation.md b/docs/plans/service-pool-relook-validation.md index 2f8fea757..c606df2d1 100644 --- a/docs/plans/service-pool-relook-validation.md +++ b/docs/plans/service-pool-relook-validation.md @@ -111,8 +111,74 @@ It covers search shrinking and expanding, repeated selection and deselection, mobile-to-desktop resizing, and Escape/focus restoration. The 31 focused tests, TypeScript, scoped lint, and production build passed after this layout change. +## Grouped selector and routing controls + +The grouped selector follow-up includes PR #1732 and is rebased onto +`ef830db7` from `main`, including the settled-attempt lease fix from PR #1739. Candidate metadata identifies +the original catalog service while +preserving each connection's own label. Group identity uses the catalog ID, +including when multiple services have the same display name; custom connections +have a separate group. Search includes original service names and slugs before +connection pagination. Metadata reads retain owner and caller scopes and do not +materialize credentials. + +The form exposes weighted and round-robin cycle ordering, configured weighted +shares, fallback priorities, and same-priority balancing. These controls use the +existing runtime contract. The independent real HTTP/CLI/browser smoke passed +20 checks, including weighted A → A → B, reordered B → A → A, saving midway +through a cycle without resetting its counter, round-robin ordering, +disabled-member skipping, actual dispatch from UI-saved configurations, and the +earlier unavailable-credential behavior. Browser checks create, edit, reopen, +and delete pools, with zero window, page, or console errors. + +Review reproduced a ResizeObserver error when opening Routing during the pool +dialog's entrance animation (39 errors across 30 fresh dialogs). Disabling only +this editor's entrance animation removed the race; the shared dialog and select +components keep their existing behavior. The permanent browser suite covers +fresh dialogs, repeated selection, resizing, focus restoration, and short +viewports. Independent production checks passed 30 fresh-dialog cycles and seven +viewport sizes, including 780×390. Each short viewport fits a complete connection +row inside the actual scroll area, with no runtime errors or horizontal overflow. + +On the rebased branch, all 4,118 frontend tests in 412 files, all three permanent +production-browser regressions, lint, the production build, Rust formatting, +and the backend boundary check passed. An initial parallel run timed out in one +signup test; the complete 18-test signup file and then the entire frontend suite +passed with bounded concurrency, without changing the test or its timeout. +The 30 focused pool component tests also passed. The extended backend inspection +regression passes on MongoDB 8 and verifies literal catalog-name/slug matching, +pagination, foreign-owner exclusion, service scope, and no credential +materialization. + ## Verification scope The local backend runs target service pools and their integration boundaries; they are not a claim that every backend test ran locally. The PR's CI jobs run the full selected backend, CLI, frontend, feature, and coverage suites. An early local run used unsupported MongoDB 7 and failed during billing bulk writes. That run was discarded and repeated on MongoDB 8. Browser interaction failures found during review were fixed and rerun on stable source. Temporary test identities, signing keys, databases, and servers were cleaned up after the successful runs. + + +## Independent review follow-up + +GPT-6-astra and Claude Opus 5.5, both at xhigh, independently reviewed issue #1680 +and PR #1737. Their findings prompted corrections for paginated selection under +latency, hidden invalid weights, same-priority member ordering, connection renames, +revision-conflict recovery, routing draft preservation, weighted cycle descriptions, +keyboard focus after final pagination, and Unicode validation parity. The backend +inspection tests were split to keep the extended module within the contributing +guide's file-length limit. + +After rebasing onto `d9a53135` from `main`, the full frontend suite passed 4,246 tests +in 429 files. The production build and lint passed with no lint errors or new +warnings in changed files. Five permanent Playwright tests passed against the +production build, including 200 ms candidate responses, two loaded pages, +repeated selection/deselection without scroll loss, focus after final pagination, +and selection changes during pagination and cached searches. The final pagination +corrections also passed 53 focused frontend tests. Candidate pages track the draft +used for compatibility checks; stale options cannot be added, and a pending next +page finishes before all loaded pages refresh. Error/retry coverage ensures failed +refreshes do not loop. All 31 CLI pool tests passed on the rebased code, and the +dedicated Rust Unicode boundary regression passed. +The fresh MongoDB 8 run passed all 11 inspection cases, including renaming through +the key service, matching the Services view, unavailable keys, explicit platform +bindings with retained keys, foreign references, selected/health views and zero +credential materialization, last-used writes or provider dispatch during inspection. diff --git a/frontend/e2e/service-pool-picker.spec.ts b/frontend/e2e/service-pool-picker.spec.ts index ad49a1231..5d6729ce9 100644 --- a/frontend/e2e/service-pool-picker.spec.ts +++ b/frontend/e2e/service-pool-picker.spec.ts @@ -17,14 +17,36 @@ const candidates = ids.map((id, index) => ({ : "compatibility_declaration_required", credential_binding: index === 2 ? "user" : "none", protocol: null, - catalog_service_id: null, + catalog_service_id: index === 2 ? "catalog-failed" : "catalog-shared", + group_name: "Shared service", + group_slug: index === 2 ? "failed-service" : "shared-service", requires_compatibility_declaration: true, cooldown_until: null, consecutive_failures: 0, last_status: null, })); +const manyCandidates = Array.from({ length: 12 }, (_, index) => ({ + ...candidates[0]!, + user_service_id: `44444444-4444-4444-8444-${String(index + 1).padStart(12, "0")}`, + name: `Responsive member ${index + 1}`, + slug: `responsive-member-${index + 1}`, + catalog_service_id: `catalog-${index % 3}`, + group_name: `Service group ${(index % 3) + 1}`, + group_slug: `service-group-${(index % 3) + 1}`, + eligible: true, + reason: null, + requires_compatibility_declaration: false, +})); -async function mockConnections(page: Page) { +async function mockConnections( + page: Page, + sourceRows = candidates, + options: { + pageSize?: number; + delayMs?: number; + peerSensitive?: boolean; + } = {}, +) { await page.route("**/api/v1/**", async (route) => { const url = new URL(route.request().url()); const path = url.pathname.replace("/api/v1", ""); @@ -46,15 +68,36 @@ async function mockConnections(page: Page) { else if (path.endsWith("/candidates")) { const search = (url.searchParams.get("search") ?? "").toLowerCase(); const peers = (url.searchParams.get("peer_ids") ?? "").split(","); + const selectedOnly = url.searchParams.get("selected_only") === "true"; + const filtered = sourceRows.filter((row) => + url.searchParams.get("selected_only") === "true" + ? peers.includes(row.user_service_id) + : row.name.toLowerCase().includes(search) || + row.slug.includes(search) || + row.group_name.toLowerCase().includes(search) || + row.group_slug.includes(search), + ); + if (!selectedOnly && options.delayMs) + await new Promise((resolve) => setTimeout(resolve, options.delayMs)); + const offset = selectedOnly + ? 0 + : Number(url.searchParams.get("after") ?? 0); + const limit = selectedOnly + ? filtered.length + : (options.pageSize ?? filtered.length); + const hasMore = offset + limit < filtered.length; body = { - candidates: candidates.filter((row) => - url.searchParams.get("selected_only") === "true" - ? peers.includes(row.user_service_id) - : row.name.toLowerCase().includes(search) || - row.slug.includes(search), - ), - has_more: false, - next_cursor: null, + candidates: filtered + .slice(offset, offset + limit) + .map((row) => + options.peerSensitive && + peers.some(Boolean) && + !peers.includes(row.user_service_id) + ? { ...row, eligible: false, reason: "incompatible_protocol" } + : row, + ), + has_more: hasMore, + next_cursor: hasMore ? String(offset + limit) : null, operation_checked: false, method: null, path: null, @@ -119,6 +162,16 @@ test("pool multi-select survives responsive resize and changing member cards wit await expect( picker.getByRole("option", { name: "Failed connection", exact: true }), ).toHaveAttribute("aria-disabled", "true"); + await expect( + picker.getByRole("group", { + name: "Shared service (shared-service) (2 loaded)", + }), + ).toBeVisible(); + await expect( + picker.getByRole("group", { + name: "Shared service (failed-service) (1 loaded)", + }), + ).toBeVisible(); await page.screenshot({ path: testInfo.outputPath("picker-desktop.png"), animations: "disabled", @@ -187,3 +240,250 @@ test("pool multi-select survives responsive resize and changing member cards wit ), ).toEqual([]); }); + +test("opening routing in a fresh pool dialog does not produce observer errors", async ({ + page, +}) => { + const runtimeErrors: string[] = []; + await page.addInitScript(() => { + const errors: string[] = []; + Object.assign(window, { poolLayoutErrors: errors }); + window.addEventListener( + "error", + (event) => { + if (event.message) errors.push(event.message); + }, + true, + ); + }); + page.on("pageerror", (error) => runtimeErrors.push(error.message)); + page.on("console", (entry) => { + if (entry.type() === "error") runtimeErrors.push(entry.text()); + }); + await mockConnections(page); + await page.goto("/keys?tab=pools"); + + for (let cycle = 0; cycle < 10; cycle++) { + await page.getByRole("button", { name: /^Create pool$/i }).click(); + const form = page.getByRole("dialog", { + name: "Create service pool", + exact: true, + }); + await form.getByLabel("Name", { exact: true }).fill(`Routing ${cycle}`); + await form.getByRole("combobox", { name: "Routing" }).click(); + await page + .getByRole("option", { name: "Weighted balancing", exact: true }) + .click(); + await form.getByRole("button", { name: "Close" }).click(); + await expect(form).toBeHidden(); + } + + expect(runtimeErrors).toEqual([]); + expect( + await page.evaluate( + () => + (window as Window & { poolLayoutErrors: string[] }).poolLayoutErrors, + ), + ).toEqual([]); +}); + +test("keeps a long picker within short landscape and mobile viewports", async ({ + page, +}) => { + const runtimeErrors: string[] = []; + await page.addInitScript(() => { + const errors: string[] = []; + Object.assign(window, { poolLayoutErrors: errors }); + window.addEventListener( + "error", + (event) => { + if (event.message) errors.push(event.message); + }, + true, + ); + }); + page.on("pageerror", (error) => runtimeErrors.push(error.message)); + page.on("console", (entry) => { + if (entry.type() === "error") runtimeErrors.push(entry.text()); + }); + await mockConnections(page, manyCandidates); + await page.setViewportSize({ width: 1024, height: 600 }); + await page.goto("/keys?tab=pools"); + await page.getByRole("button", { name: /^Create pool$/i }).click(); + const form = page.getByRole("dialog", { + name: "Create service pool", + exact: true, + }); + await form.getByLabel("Name", { exact: true }).fill("Short viewport pool"); + await form.getByRole("button", { name: "Choose connections" }).click(); + const picker = page.getByRole("dialog", { + name: "Choose pool connections", + exact: true, + }); + await expect(picker.getByRole("option")).toHaveCount(12); + + for (const [width, height] of [ + [1024, 600], + [780, 390], + [390, 480], + ] as const) { + await page.setViewportSize({ width, height }); + await expect + .poll(async () => { + const box = await picker.boundingBox(); + return Boolean( + box && + box.x >= 0 && + box.y >= 0 && + box.x + box.width <= width + 1 && + box.y + box.height <= height + 1, + ); + }) + .toBe(true); + expect( + await picker.evaluate((node) => node.scrollWidth <= node.clientWidth + 1), + ).toBe(true); + const firstOption = picker.getByRole("option").first(); + const scrollViewport = picker.getByRole("listbox").locator(".."); + await firstOption.scrollIntoViewIfNeeded(); + await expect + .poll(async () => { + const listBox = await scrollViewport.boundingBox(); + const optionBox = await firstOption.boundingBox(); + return Boolean( + listBox && + optionBox && + optionBox.y >= listBox.y - 1 && + optionBox.y + optionBox.height <= listBox.y + listBox.height + 1, + ); + }) + .toBe(true); + } + expect(runtimeErrors).toEqual([]); + expect( + await page.evaluate( + () => + (window as Window & { poolLayoutErrors: string[] }).poolLayoutErrors, + ), + ).toEqual([]); +}); + +test("selection preserves delayed inventory pages and scroll, and final pagination restores keyboard focus", async ({ + page, +}) => { + const rows = Array.from({ length: 40 }, (_, index) => ({ + ...manyCandidates[0]!, + user_service_id: `55555555-5555-4555-8555-${String(index + 1).padStart(12, "0")}`, + name: `Account ${index + 1}`, + slug: `account-${index + 1}`, + })); + await mockConnections(page, rows, { pageSize: 20, delayMs: 200 }); + const errors: string[] = []; + page.on("pageerror", (error) => errors.push(error.message)); + await page.goto("/keys?tab=pools"); + await page.getByRole("button", { name: /^Create pool$/i }).click(); + await page.getByRole("button", { name: "Choose connections" }).click(); + const picker = page.getByRole("dialog", { + name: "Choose pool connections", + exact: true, + }); + const search = picker.getByRole("combobox"); + await expect(picker.getByRole("option")).toHaveCount(20); + await search.press("Tab"); + await expect( + picker.getByRole("button", { name: "Load more connections" }), + ).toBeFocused(); + await page.keyboard.press("Enter"); + await expect(picker.getByRole("option")).toHaveCount(40); + await expect(search).toBeFocused(); + const last = picker.getByRole("option", { name: "Account 40", exact: true }); + await last.scrollIntoViewIfNeeded(); + const scrollTop = () => + picker + .getByRole("listbox") + .evaluate((node) => node.parentElement!.scrollTop); + const before = await scrollTop(); + expect(before).toBeGreaterThan(0); + for (const selected of ["true", "false", "true"]) { + const refreshedLastPage = page.waitForResponse((response) => { + const url = new URL(response.url()); + return ( + url.pathname.endsWith("/candidates") && + url.searchParams.get("after") === "20" + ); + }); + await last.click(); + await expect(last).toHaveAttribute("aria-selected", selected); + await expect(picker.getByRole("option")).toHaveCount(40); + await refreshedLastPage; + await expect(picker.getByRole("option")).toHaveCount(40); + await expect.poll(scrollTop).toBe(before); + await expect(search).toBeFocused(); + } + expect(errors).toEqual([]); +}); + +test("pagination and selection finish without stale options or dropped pages", async ({ + page, +}) => { + const rows = manyCandidates.slice(0, 8).map((row) => ({ + ...row, + catalog_service_id: "shared", + group_name: "Shared service", + group_slug: "shared", + })); + await mockConnections(page, rows, { + pageSize: 3, + delayMs: 500, + peerSensitive: true, + }); + await page.goto("/keys?tab=pools"); + await page.getByRole("button", { name: /^Create pool$/i }).click(); + await page.getByRole("button", { name: "Choose connections" }).click(); + const picker = page.getByRole("dialog", { + name: "Choose pool connections", + exact: true, + }); + const primary = picker.getByRole("option", { + name: rows[0]!.name, + exact: true, + }); + const second = picker.getByRole("option", { + name: rows[1]!.name, + exact: true, + }); + const more = picker.getByRole("button", { name: "Load more connections" }); + await expect(picker.getByRole("option")).toHaveCount(3); + await more.click(); + await primary.click(); + await expect(second).toHaveAttribute("aria-disabled", "true"); + await expect(picker.getByRole("listbox")).toHaveAttribute( + "aria-busy", + "false", + ); + await expect(picker.getByRole("option")).toHaveCount(6); + await expect(second).toHaveAttribute("aria-disabled", "true"); + await primary.click(); + await expect(more).toBeDisabled(); + await expect(second).toHaveAttribute("aria-disabled", "true"); + await expect(second).toHaveAttribute("aria-disabled", "false"); + await expect(more).toBeEnabled(); + await more.click(); + await expect(picker.getByRole("option")).toHaveCount(8); + const search = picker.getByRole("combobox"); + await search.fill(rows[0]!.name); + await expect(picker.getByRole("option")).toHaveCount(1); + await primary.click(); + await expect(picker.getByRole("listbox")).toHaveAttribute( + "aria-busy", + "false", + ); + await search.clear(); + await expect(picker.getByRole("option")).toHaveCount(8); + await expect(second).toHaveAttribute("aria-disabled", "true"); + await expect(picker.getByRole("listbox")).toHaveAttribute( + "aria-busy", + "false", + ); + await expect(second).toHaveAttribute("aria-disabled", "true"); +}); diff --git a/frontend/src/components/dashboard/pool-connection-picker.test.tsx b/frontend/src/components/dashboard/pool-connection-picker.test.tsx index d42adbc6e..0183b4b9a 100644 --- a/frontend/src/components/dashboard/pool-connection-picker.test.tsx +++ b/frontend/src/components/dashboard/pool-connection-picker.test.tsx @@ -53,7 +53,13 @@ const defaults = { isFetchingNextPage: false, onLoadMore: vi.fn(), }; -function Harness({ initialIds = [] }: { initialIds?: string[] }) { +function Harness({ + initialIds = [], + sourceRows = defaults.rows, +}: { + initialIds?: string[]; + sourceRows?: PoolCandidate[]; +}) { const [ids, setIds] = useState(initialIds); const [search, setSearch] = useState(""); return ( @@ -66,8 +72,10 @@ function Harness({ initialIds = [] }: { initialIds?: string[] }) { selectedIds={ids} search={search} onSearch={setSearch} - rows={defaults.rows.filter((row) => - (row.name || row.slug).toLowerCase().includes(search.toLowerCase()), + rows={sourceRows.filter((row) => + `${row.name || row.slug} ${row.slug} ${row.group_name ?? ""}` + .toLowerCase() + .includes(search.toLowerCase()), )} onToggle={(row) => setIds((current) => @@ -190,6 +198,74 @@ describe("pool connection picker", () => { "true", ); }); + it("groups interleaved catalog rows, de-duplicates page overlap, and navigates display order", async () => { + const user = userEvent.setup(); + const groupedRows: PoolCandidate[] = [ + { + ...candidate, + name: "A account 1", + catalog_service_id: "catalog-a", + group_name: "Shared service", + group_slug: "service-a", + }, + { + ...backup, + name: "B account 1", + catalog_service_id: "catalog-b", + group_name: "Shared service", + group_slug: "service-b", + }, + { + ...candidate, + user_service_id: "three", + name: "A account 2", + catalog_service_id: "catalog-a", + group_name: "Shared service", + group_slug: "service-a", + }, + { + ...candidate, + user_service_id: "three", + name: "A account 2 duplicate", + catalog_service_id: "catalog-a", + group_name: "Shared service", + group_slug: "service-a", + }, + ]; + render(); + const { input } = await openPicker(user); + expect( + screen.getByRole("group", { + name: "Shared service (service-a) (2 loaded)", + }), + ).toBeVisible(); + expect( + screen.getByRole("group", { + name: "Shared service (service-b) (1 loaded)", + }), + ).toBeVisible(); + expect(screen.getAllByRole("option")).toHaveLength(3); + await user.keyboard("{ArrowUp}"); + expect(input).toHaveAttribute( + "aria-activedescendant", + screen.getByRole("option", { name: "B account 1" }).id, + ); + await user.keyboard("{Enter}"); + expect(screen.getByRole("option", { name: "B account 1" })).toHaveAttribute( + "aria-selected", + "true", + ); + await user.keyboard("{ArrowDown}{Enter}{ArrowDown}{Enter}"); + expect(screen.getByRole("option", { name: "A account 1" })).toHaveAttribute( + "aria-selected", + "true", + ); + expect(screen.getByRole("option", { name: "A account 2" })).toHaveAttribute( + "aria-selected", + "true", + ); + expect(input).toHaveFocus(); + }); it("keeps retry and paging inside the dropdown and distinguishes loading, empty search, and empty inventory", async () => { const user = userEvent.setup(); const retry = vi.fn(); @@ -230,3 +306,75 @@ describe("pool connection picker", () => { expect(screen.getByRole("combobox")).toBeVisible(); }); }); + +it("returns keyboard focus to search after the final page button disappears", async () => { + const user = userEvent.setup(); + const { rerender } = render( + , + ); + const { input } = await openPicker(user); + await user.tab(); + const more = screen.getByRole("button", { name: "Load more connections" }); + expect(more).toHaveFocus(); + await user.keyboard("{Enter}"); + rerender( + , + ); + expect(more).toBeDisabled(); + rerender( + , + ); + await waitFor(() => expect(input).toHaveFocus()); +}); + +it("blocks stale additions and pagination during compatibility checks while allowing removal", async () => { + const user = userEvent.setup(); + const onToggle = vi.fn(); + const onLoadMore = vi.fn(); + const { rerender } = render( + , + ); + await openPicker(user); + expect(screen.getByText("Checking compatibility…")).toBeVisible(); + const primary = screen.getByRole("option", { name: candidate.name! }); + const second = screen.getByRole("option", { name: backup.name }); + expect(primary).toHaveAttribute("aria-disabled", "false"); + expect(second).toHaveAttribute("aria-disabled", "true"); + await user.click(second); + await user.click( + screen.getByRole("button", { name: "Load more connections" }), + ); + expect(onToggle).not.toHaveBeenCalled(); + expect(onLoadMore).not.toHaveBeenCalled(); + await user.click(primary); + expect(onToggle).toHaveBeenCalledWith(candidate); + rerender( + , + ); + expect(screen.getByText("Compatibility check failed.")).toBeVisible(); + expect(screen.queryByText("Checking compatibility…")).not.toBeInTheDocument(); + rerender( + , + ); + await user.click( + screen.getByRole("button", { name: "Load more connections" }), + ); + expect(onLoadMore).toHaveBeenCalledTimes(1); +}); diff --git a/frontend/src/components/dashboard/pool-connection-picker.tsx b/frontend/src/components/dashboard/pool-connection-picker.tsx index 4982bba65..1cf4aa1f0 100644 --- a/frontend/src/components/dashboard/pool-connection-picker.tsx +++ b/frontend/src/components/dashboard/pool-connection-picker.tsx @@ -1,6 +1,7 @@ -import { useEffect, useId, useRef, useState } from "react"; +import { useEffect, useId, useMemo, useRef, useState } from "react"; import { Check, ChevronsUpDown } from "lucide-react"; import { ErrorBanner } from "@/components/shared/error-banner"; +import { ServiceIcon } from "@/components/service-icon"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { @@ -19,6 +20,8 @@ type Props = { onToggle: (row: PoolCandidate) => void; isLoading: boolean; isSearching: boolean; + isCheckingCompatibility?: boolean; + isRefreshing?: boolean; isError: boolean; error: unknown; onRetry: () => void; @@ -35,6 +38,8 @@ export function PoolConnectionPicker({ onToggle, isLoading, isSearching, + isCheckingCompatibility = false, + isRefreshing = false, isError, error, onRetry, @@ -48,7 +53,57 @@ export function PoolConnectionPicker({ const listId = `${id}-list`; const input = useRef(null); const list = useRef(null); - const activeIndex = rows.findIndex((row) => row.user_service_id === activeId); + const loadMoreButton = useRef(null); + const restoreLoadMoreFocus = useRef(false); + useEffect(() => { + if (isFetchingNextPage || !restoreLoadMoreFocus.current) return; + restoreLoadMoreFocus.current = false; + if (document.activeElement === document.body) + (hasNextPage ? loadMoreButton.current : input.current)?.focus(); + }, [isFetchingNextPage, hasNextPage, rows]); + const groups = useMemo(() => { + const grouped = new Map< + string, + { + key: string; + name: string; + slug: string | null; + rows: PoolCandidate[]; + } + >(); + const seen = new Set(); + for (const row of rows) { + if (seen.has(row.user_service_id)) continue; + seen.add(row.user_service_id); + const key = row.catalog_service_id ?? "custom"; + const existing = grouped.get(key); + if (existing) { + existing.rows.push(row); + continue; + } + grouped.set(key, { + key, + name: + row.group_name?.trim() || + (row.catalog_service_id ? "Catalog service" : "Custom connections"), + slug: row.group_slug ?? null, + rows: [row], + }); + } + return [...grouped.values()]; + }, [rows]); + const displayRows = useMemo( + () => groups.flatMap((group) => group.rows), + [groups], + ); + const displayIndex = useMemo( + () => + new Map(displayRows.map((row, index) => [row.user_service_id, index])), + [displayRows], + ); + const activeIndex = displayRows.findIndex( + (row) => row.user_service_id === activeId, + ); const busy = isLoading || isSearching; useEffect(() => { if (activeIndex >= 0) @@ -59,7 +114,8 @@ export function PoolConnectionPicker({ function disabled(row: PoolCandidate) { return ( !selectedIds.includes(row.user_service_id) && - (selectedIds.length >= 50 || + (isCheckingCompatibility || + selectedIds.length >= 50 || (!row.eligible && row.reason !== "compatibility_declaration_required")) ); } @@ -68,14 +124,14 @@ export function PoolConnectionPicker({ input.current?.focus(); } function navigate(direction: number) { - if (!rows.length) return; + if (!displayRows.length) return; const next = activeIndex < 0 ? direction > 0 ? 0 - : rows.length - 1 - : (activeIndex + direction + rows.length) % rows.length; - setActiveId(rows[next]!.user_service_id); + : displayRows.length - 1 + : (activeIndex + direction + displayRows.length) % displayRows.length; + setActiveId(displayRows[next]!.user_service_id); } return (
@@ -99,7 +155,7 @@ export function PoolConnectionPicker({ align="start" collisionPadding={12} aria-label="Choose pool connections" - className="flex max-h-[min(28rem,var(--radix-popover-content-available-height))] w-[var(--radix-popover-trigger-width)] max-w-[calc(100vw-1.5rem)] flex-col gap-2 p-2 data-[state=closed]:hidden data-[state=closed]:animate-none" + className="flex max-h-[min(28rem,calc(50dvh-2rem))] w-[var(--radix-popover-trigger-width)] max-w-[calc(100vw-1.5rem)] flex-col gap-2 p-2 [@media(max-height:500px)]:gap-1 [@media(max-height:500px)]:p-1.5 data-[state=closed]:hidden data-[state=closed]:animate-none" onOpenAutoFocus={(event) => { event.preventDefault(); input.current?.focus(); @@ -136,12 +192,19 @@ export function PoolConnectionPicker({ navigate(event.key === "ArrowDown" ? 1 : -1); } else if (event.key === "Enter") { event.preventDefault(); - if (activeIndex >= 0) choose(rows[activeIndex]!); + if (activeIndex >= 0) choose(displayRows[activeIndex]!); } }} /> -

- Select multiple connections. Select again to remove. +

+ {isCheckingCompatibility + ? isError + ? "Compatibility check failed." + : "Checking compatibility…" + : "Select multiple connections. Select again to remove."}

{isError && ( @@ -163,51 +226,78 @@ export function PoolConnectionPicker({ role="listbox" aria-label="Connections" aria-multiselectable="true" - aria-busy={busy} + aria-busy={busy || isCheckingCompatibility} > - {rows.map((row, index) => { - const selected = selectedIds.includes(row.user_service_id); - const unavailable = disabled(row); - return ( -
event.preventDefault()} - onClick={() => choose(row)} - > -
+ ))}
{!busy && !isError && rows.length === 0 && (

{ + restoreLoadMoreFocus.current = + document.activeElement === event.currentTarget; + void onLoadMore(); + }} > Load more connections diff --git a/frontend/src/components/dashboard/pool-connections-editor.tsx b/frontend/src/components/dashboard/pool-connections-editor.tsx index b50beb5b7..a1ea254ef 100644 --- a/frontend/src/components/dashboard/pool-connections-editor.tsx +++ b/frontend/src/components/dashboard/pool-connections-editor.tsx @@ -1,5 +1,6 @@ import { useEffect, useState } from "react"; import { useWatch, type UseFormReturn } from "react-hook-form"; +import { ArrowDown, ArrowUp } from "lucide-react"; import { ErrorBanner } from "@/components/shared/error-banner"; import { Button } from "@/components/ui/button"; import { Checkbox } from "@/components/ui/checkbox"; @@ -11,7 +12,9 @@ import type { ServicePool, ServicePoolMember, } from "@/schemas/pools"; -import { NumberInput, Toggle } from "./pool-controls"; +import { PoolCycleSummary } from "./pool-cycle-summary"; +import { validPoolWeight } from "./pool-editor-state"; +import { Choice, NumberInput, Toggle } from "./pool-controls"; import { bindingLabel, reason } from "./pool-labels"; import { PoolConnectionPicker } from "./pool-connection-picker"; import { PoolOperationCheck, type PoolOperation } from "./pool-operation-check"; @@ -56,7 +59,7 @@ export function PoolConnectionsEditor({ const inspection = { poolId: pool?.id, orgId, - contract: values.member_contract, + contract: aiChat ? ("ai_chat" as const) : ("same_api" as const), strategy: values.strategy, checkOperation: !aiChat && operation !== null, method: aiChat ? undefined : operation?.method, @@ -92,6 +95,46 @@ export function PoolConnectionsEditor({ savedMembers.data?.candidates.map((row) => [row.user_service_id, row]) ?? [], ); + function memberLabel(id: string, fallbackIndex: number) { + const row = selectedRows.get(id) ?? selectedLabels[id] ?? savedRows.get(id); + return row?.name || row?.slug || `Connection ${fallbackIndex + 1}`; + } + function validPriority(value: number | undefined) { + return ( + value === undefined || + (Number.isFinite(value) && + Number.isInteger(value) && + value >= 0 && + value <= 4294967295) + ); + } + function configuredShare(member: Partial): string | null { + const memberWeight = member.weight ?? 1; + if (member.enabled === false || !validPoolWeight(memberWeight)) return null; + const memberPriority = member.priority ?? 0; + if (priority && !validPriority(memberPriority)) return null; + const shareMembers = + priority && values.tier_balance === "weighted" + ? members.filter( + (candidate) => + candidate.enabled !== false && + (candidate.priority ?? 0) === memberPriority, + ) + : members.filter((candidate) => candidate.enabled !== false); + if ( + shareMembers.some((candidate) => !validPoolWeight(candidate.weight ?? 1)) + ) + return null; + const total = shareMembers.reduce( + (sum, candidate) => sum + (candidate.weight ?? 1), + 0, + ); + if (!Number.isFinite(total) || total <= 0) return null; + const share = (memberWeight / total) * 100; + if (!Number.isFinite(share) || share < 0) return null; + if (share > 0 && share < 0.1) return "<0.1%"; + return Number.isInteger(share) ? `${share}%` : `${share.toFixed(1)}%`; + } function setMember(id: string, patch: Partial) { form.setValue( "members", @@ -131,6 +174,26 @@ export function PoolConnectionsEditor({ { ...newMember(row.user_service_id), priority: nextPriority }, ]); } + function moveMember(id: string, direction: -1 | 1) { + const current = form.getValues("members"); + const index = current.findIndex((member) => member.user_service_id === id); + if (index < 0) return; + const peers = current + .map((member, originalIndex) => ({ member, originalIndex })) + .filter( + ({ member }) => + !priority || + (member.priority ?? 0) === (current[index]!.priority ?? 0), + ); + const peerPosition = peers.findIndex( + ({ originalIndex }) => originalIndex === index, + ); + const target = peers[peerPosition + direction]?.originalIndex; + if (target === undefined) return; + const next: ServicePoolMember[] = [...current]; + [next[index], next[target]] = [next[target]!, next[index]!]; + form.setValue("members", next); + } const orderedMembers = members .map((member, index) => ({ member, index })) .sort((a, b) => @@ -150,6 +213,29 @@ export function PoolConnectionsEditor({ : "Add the connections that should share traffic."}

+ {priority && ( +
+ { + form.setValue( + "tier_balance", + value as "round_robin" | "weighted", + ); + void form.trigger(); + }} + /> +

+ Lower priority numbers are tried first. Ties use this balancing + mode; unavailable members can still be skipped at request time. +

+
+ )}

Select connections

{ @@ -170,9 +255,7 @@ export function PoolConnectionsEditor({ }} hasNextPage={candidates.hasNextPage} isFetchingNextPage={candidates.isFetchingNextPage} - onLoadMore={() => { - void candidates.fetchNextPage(); - }} + onLoadMore={() => candidates.fetchNextPage({ cancelRefetch: false })} />
{members.length === 0 && ( @@ -184,6 +267,14 @@ export function PoolConnectionsEditor({ You can mix platform access and your own keys. )} + {members.length > 0 && ( + + )} {selected.isError && ( )} - {orderedMembers.map(({ member }, position) => { + {orderedMembers.map(({ member, index: originalIndex }, position) => { const id = member.user_service_id!; const candidate = selectedRows.get(id); const labelRow = candidate ?? selectedLabels[id] ?? savedRows.get(id); @@ -201,6 +292,23 @@ export function PoolConnectionsEditor({ priority && (labelRow?.requires_compatibility_declaration || member.same_api_compatible); + const share = configuredShare(member); + const invalidPriority = priority && !validPriority(member.priority); + const tierMembers = orderedMembers.filter( + ({ member: peer }) => + !priority || (peer.priority ?? 0) === (member.priority ?? 0), + ); + const tierPosition = tierMembers.findIndex( + ({ member: peer }) => peer.user_service_id === id, + ); + const enabledTier = tierMembers.filter( + ({ member: peer }) => peer.enabled !== false, + ); + const cyclePosition = enabledTier.findIndex( + ({ member: peer }) => peer.user_service_id === id, + ); + const weightError = + form.formState.errors.members?.[originalIndex]?.weight?.message; return (
+ {!invalidPriority && ( +

+ {member.enabled === false ? ( + "Disabled · excluded from cycle" + ) : ( + <> + {priority ? "Tier cycle position" : "Cycle position"}{" "} + {cyclePosition + 1} of {enabledTier.length} + {cyclePosition === 0 ? " · first in cycle" : ""} + {cyclePosition === enabledTier.length - 1 + ? " · last in cycle" + : ""} + + )} + {!priority && weighted + ? member.enabled === false + ? " · excluded from configured share" + : share + ? ` · configured share ${share}` + : " · configured share unavailable until enabled weights are valid" + : ""} +

+ )} + {priority && values.tier_balance === "weighted" && ( +

+ {invalidPriority + ? "Priority tier needs a valid number · configured share unavailable" + : `Priority tier ${member.priority ?? 0} · ${ + share + ? `configured tier share ${share}` + : member.enabled === false + ? "disabled · excluded from tier share" + : "configured share unavailable until enabled weights in this tier are valid" + }`} +

+ )} {candidate?.reason && (

{reason(candidate)} @@ -243,6 +387,32 @@ export function PoolConnectionsEditor({ > Remove + {(!priority || !invalidPriority) && ( +

+ + +
+ )}
)}
+ {weighted && weightError && ( +

+ {memberLabel(id, position)}: {weightError} +

+ )} onChange(e.target.valueAsNumber)} /> diff --git a/frontend/src/components/dashboard/pool-cycle-summary.tsx b/frontend/src/components/dashboard/pool-cycle-summary.tsx new file mode 100644 index 000000000..81065d435 --- /dev/null +++ b/frontend/src/components/dashboard/pool-cycle-summary.tsx @@ -0,0 +1,69 @@ +import type { ServicePoolMember } from "@/schemas/pools"; +import { validPoolWeight } from "./pool-editor-state"; + +export function PoolCycleSummary({ + members, + priority, + weighted, + label, +}: { + members: Partial[]; + priority: boolean; + weighted: boolean; + label: (id: string, index: number) => string; +}) { + const groups = new Map< + number, + { member: Partial; index: number }[] + >(); + members.forEach((member, index) => { + if (member.enabled === false) return; + const tier = priority ? (member.priority ?? 0) : 0; + if (!Number.isInteger(tier) || tier < 0 || tier > 4294967295) return; + const group = groups.get(tier) ?? []; + group.push({ member, index }); + groups.set(tier, group); + }); + return ( +
+

+ {priority + ? "Cycles within each priority" + : weighted + ? "Repeating weighted cycle" + : "Repeating round-robin cycle"} +

+

+ {weighted + ? "Each enabled connection takes consecutive turns equal to its weight, in the saved order." + : "Each enabled connection gets one turn in the saved order."}{" "} + Unavailable connections can be skipped. + {priority + ? " Lower priority numbers are tried first. Saving restarts each tier’s cycle." + : " The current position is retained between requests and when saving; this does not promise a next or fallback connection."} +

+ {groups.size === 0 && ( +

+ {priority + ? "No enabled connections with a valid priority." + : "No enabled connections."} +

+ )} + {[...groups] + .sort(([a], [b]) => a - b) + .map(([tier, group]) => ( +

+ {priority ? `Priority ${tier} cycle order: ` : "Cycle order: "} + {group + .map(({ member, index }) => { + const name = label(member.user_service_id!, index); + if (!weighted) return name; + const weight = member.weight ?? 1; + return `${name} (${validPoolWeight(weight) ? `${weight} ${weight === 1 ? "turn" : "turns"}` : "weight required"})`; + }) + .join(" → ")} +

+ ))} +
+ ); +} diff --git a/frontend/src/components/dashboard/pool-editor-state.ts b/frontend/src/components/dashboard/pool-editor-state.ts new file mode 100644 index 000000000..82b9096dd --- /dev/null +++ b/frontend/src/components/dashboard/pool-editor-state.ts @@ -0,0 +1,48 @@ +import type { CreateServicePoolInput, ServicePool } from "@/schemas/pools"; + +export function poolEditorDefaults(pool?: ServicePool): CreateServicePoolInput { + return { + slug: pool?.slug ?? "", + name: pool?.name ?? "", + description: pool?.description ?? "", + strategy: pool?.strategy ?? "priority", + tier_balance: pool?.tier_balance ?? "round_robin", + member_contract: pool?.member_contract ?? "same_api", + failover: pool?.failover ?? null, + members: pool?.members ?? [], + is_active: pool?.is_active ?? true, + }; +} + +export function validPoolWeight(value: number | undefined): boolean { + return ( + value !== undefined && + Number.isInteger(value) && + value >= 1 && + value <= 1000 + ); +} + +// Keep every routing mode's draft settings; submit only the active mode's fields. +export function poolEditorPayload( + input: CreateServicePoolInput, +): CreateServicePoolInput { + const priority = input.strategy === "priority"; + const weighted = + input.strategy === "weighted" || + (priority && input.tier_balance === "weighted"); + const aiChat = priority && input.member_contract === "ai_chat"; + return { + ...input, + tier_balance: priority ? input.tier_balance : "round_robin", + member_contract: aiChat ? "ai_chat" : "same_api", + failover: priority ? input.failover : null, + members: input.members.map((member) => ({ + ...member, + weight: !weighted && !validPoolWeight(member.weight) ? 1 : member.weight, + priority: priority ? member.priority : 0, + model: aiChat ? member.model : null, + same_api_compatible: priority ? member.same_api_compatible : false, + })), + }; +} diff --git a/frontend/src/components/dashboard/pool-editor.tsx b/frontend/src/components/dashboard/pool-editor.tsx index d9133b10d..d3fafed72 100644 --- a/frontend/src/components/dashboard/pool-editor.tsx +++ b/frontend/src/components/dashboard/pool-editor.tsx @@ -3,6 +3,7 @@ import { zodResolver } from "@hookform/resolvers/zod"; import { useWatch } from "react-hook-form"; import { Check } from "lucide-react"; import { toast } from "sonner"; +import { ApiError } from "@/lib/api-client"; import { firstNestedErrorMessage } from "@/lib/form-errors"; import { ErrorBanner } from "@/components/shared/error-banner"; import { Button } from "@/components/ui/button"; @@ -25,7 +26,11 @@ import { FormMessage, } from "@/components/ui/form"; import { Input } from "@/components/ui/input"; -import { useCreateServicePool, useUpdateServicePool } from "@/hooks/use-pools"; +import { + useCreateServicePool, + useUpdateServicePool, + useReloadServicePool, +} from "@/hooks/use-pools"; import { createServicePoolSchema, defaultFailoverPolicy, @@ -33,6 +38,7 @@ import { type FailoverPolicy, type ServicePool, } from "@/schemas/pools"; +import { poolEditorDefaults, poolEditorPayload } from "./pool-editor-state"; import { PoolConnectionsEditor } from "./pool-connections-editor"; import { Choice, PolicyEditor, Toggle } from "./pool-controls"; import { message, strategyLabels } from "./pool-labels"; @@ -53,28 +59,22 @@ export function PoolEditor({ }) { const create = useCreateServicePool(); const update = useUpdateServicePool(); + const reload = useReloadServicePool(); + const [revision, setRevision] = useState(pool?.config_revision ?? 0); + const [conflict, setConflict] = useState(false); const form = useAppForm({ - resolver: zodResolver( - createServicePoolSchema.refine( - (input) => Boolean(pool) || input.members.length > 0, - { - path: ["members"], - message: "Add at least one connection to create a pool.", - }, - ), - ), + resolver: (input, context, options) => + zodResolver( + createServicePoolSchema.refine( + (input) => Boolean(pool) || input.members.length > 0, + { + path: ["members"], + message: "Add at least one connection to create a pool.", + }, + ), + )(poolEditorPayload(input), context, options), mode: "onChange", - defaultValues: { - slug: pool?.slug ?? "", - name: pool?.name ?? "", - description: pool?.description ?? "", - strategy: pool?.strategy ?? "priority", - tier_balance: pool?.tier_balance ?? "round_robin", - member_contract: pool?.member_contract ?? "same_api", - failover: pool?.failover ?? null, - members: pool?.members ?? [], - is_active: pool?.is_active ?? true, - }, + defaultValues: poolEditorDefaults(pool), }); const values = useWatch({ control: form.control }); const priority = values.strategy === "priority"; @@ -93,19 +93,18 @@ export function PoolEditor({ function setStrategy(value: string) { const strategy = value as CreateServicePoolInput["strategy"]; form.setValue("strategy", strategy); - if (strategy !== "priority") { - setContract("same_api"); - form.setValue("tier_balance", "round_robin"); - form.setValue("failover", null); - form.setValue( - "members", - form.getValues("members").map((m) => ({ - ...m, - priority: 0, - model: null, - same_api_compatible: false, - })), - ); + void form.trigger(); + } + async function reloadLatest() { + if (!pool) return; + try { + const latest = await reload.mutateAsync(pool.id); + form.reset(poolEditorDefaults(latest)); + setRevision(latest.config_revision ?? 0); + setConflict(false); + setOperation(null); + } catch (error) { + form.setError("root", { message: message(error) }); } } async function save(input: CreateServicePoolInput) { @@ -121,7 +120,7 @@ export function PoolEditor({ await update.mutateAsync({ ...normalized, poolId: pool.id, - expected_revision: pool.config_revision ?? 0, + expected_revision: revision, description: input.description?.trim() || null, }); else @@ -133,10 +132,11 @@ export function PoolEditor({ toast.success(pool ? "Service pool saved" : "Service pool created"); onClose(); } catch (error) { + if (error instanceof ApiError && error.status === 409) setConflict(true); form.setError("root", { message: message(error) }); } } - const pending = create.isPending || update.isPending; + const pending = create.isPending || update.isPending || reload.isPending; const { isDirty, isValid, errors } = form.formState; const rootError = errors.root?.message ?? firstNestedErrorMessage(errors); return ( @@ -149,7 +149,7 @@ export function PoolEditor({ @@ -303,20 +303,6 @@ export function PoolEditor({ /> {priority && ( <> - - form.setValue( - "tier_balance", - v as "round_robin" | "weighted", - ) - } - /> {rootError && } + {conflict && ( +
+

+ This pool changed elsewhere. Reload the latest settings to + replace this draft before saving. +

+ +
+ )} diff --git a/frontend/src/components/dashboard/pool-routing-controls.test.tsx b/frontend/src/components/dashboard/pool-routing-controls.test.tsx new file mode 100644 index 000000000..e7d77eb29 --- /dev/null +++ b/frontend/src/components/dashboard/pool-routing-controls.test.tsx @@ -0,0 +1,461 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { PoolEditor } from "./service-pools-tab"; +import type { PoolCandidate, ServicePool } from "@/schemas/pools"; + +const mocks = vi.hoisted(() => ({ + update: vi.fn(), + reload: vi.fn(), + create: vi.fn(), + candidates: vi.fn(), + health: vi.fn(), + reset: vi.fn(), + pools: vi.fn(), +})); +vi.mock("@/hooks/use-pools", () => ({ + useUpdateServicePool: () => ({ mutateAsync: mocks.update, isPending: false }), + useReloadServicePool: () => ({ mutateAsync: mocks.reload, isPending: false }), + useCreateServicePool: () => ({ mutateAsync: mocks.create, isPending: false }), + usePoolCandidates: mocks.candidates, + useDeleteServicePool: () => ({ isPending: false }), + usePoolHealth: mocks.health, + useResetPoolHealth: () => ({ mutateAsync: mocks.reset, isPending: false }), + useServicePools: mocks.pools, +})); +vi.mock("@/hooks/use-orgs", () => ({ useOrgs: mocks.pools })); +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +const candidate: PoolCandidate = { + user_service_id: "member-id", + name: "My connection", + slug: "my-member", + is_active: true, + credential_binding: "user", + protocol: "openai_completions", + eligible: true, + reason: null, + catalog_service_id: "catalog-id", + requires_compatibility_declaration: false, + cooldown_until: null, + consecutive_failures: 0, + last_status: null, +}; +const pool: ServicePool = { + id: "pool-id", + user_id: "owner", + slug: "ai-route", + name: "My pool", + strategy: "priority", + member_contract: "same_api", + config_revision: 17, + tier_balance: "round_robin", + failover: null, + members: [ + { + user_service_id: "member-id", + weight: 2, + enabled: true, + priority: 0, + model: null, + same_api_compatible: true, + }, + ], + rr_counter: 0, + is_active: true, + created_at: "2026-01-01", + updated_at: "2026-01-01", +}; +const page = (rows: PoolCandidate[]) => ({ + data: { pages: [{ candidates: rows, has_more: false, next_cursor: null }] }, + isLoading: false, +}); +const saveButton = () => screen.getByRole("button", { name: "Save" }); + +beforeEach(() => { + vi.clearAllMocks(); + mocks.update.mockResolvedValue(pool); + mocks.create.mockResolvedValue(pool); + mocks.pools.mockReturnValue({ data: [], isLoading: false }); + mocks.health.mockReturnValue({ + data: { + candidates: [candidate], + operation_checked: false, + method: null, + path: null, + }, + }); + mocks.reset.mockResolvedValue({ reset: true }); + mocks.candidates.mockReturnValue(page([candidate])); +}); + +describe("pool routing controls", () => { + it("shows truthful weighted shares and cycle order while editing", async () => { + const user = userEvent.setup(); + const second = { + ...candidate, + user_service_id: "second-id", + name: "Second connection", + slug: "second", + }; + mocks.candidates.mockReturnValue(page([candidate, second])); + mocks.health.mockReturnValue({ + data: { + candidates: [candidate, second], + operation_checked: false, + method: null, + path: null, + }, + }); + render( + , + ); + expect(screen.getByText(/configured share 75%/)).toBeVisible(); + expect(screen.getByText(/configured share 25%/)).toBeVisible(); + expect( + screen.getByText(/Cycle position 1 of 2 · first in cycle/), + ).toBeVisible(); + expect( + screen.getByText(/Cycle position 2 of 2 · last in cycle/), + ).toBeVisible(); + expect( + screen.getByText( + /Cycle order: My connection \(3 turns\) → Second connection \(1 turn\)/, + ), + ).toBeVisible(); + await user.click( + screen.getByRole("button", { + name: "Move connection 2 earlier in the cycle", + }), + ); + expect( + screen.getByText( + /Cycle order: Second connection \(1 turn\) → My connection \(3 turns\)/, + ), + ).toBeVisible(); + await user.clear(screen.getByLabelText("Weight for member 1")); + expect( + screen.getAllByText( + /configured share unavailable until enabled weights are valid/, + ), + ).toHaveLength(2); + expect(screen.queryByText(/NaN%|Infinity%/)).not.toBeInTheDocument(); + await user.click(screen.getByRole("switch", { name: "Member 1 enabled" })); + expect( + screen.getByText( + /Disabled · excluded from cycle · excluded from configured share/, + ), + ).toBeVisible(); + expect(screen.getByText(/configured share 100%/)).toBeVisible(); + }); + it("keeps weighted share displays neutral for invalid values and precise for tiny valid shares", async () => { + const first = { ...pool.members[0]!, weight: 1 }; + const second = { + ...pool.members[0]!, + user_service_id: "second-id", + weight: 1000, + }; + const secondRow = { + ...candidate, + user_service_id: "second-id", + name: "Second connection", + slug: "second", + }; + mocks.candidates.mockReturnValue(page([candidate, secondRow])); + mocks.health.mockReturnValue({ + data: { + candidates: [candidate, secondRow], + operation_checked: false, + method: null, + path: null, + }, + }); + const user = userEvent.setup(); + render( + , + ); + expect(screen.getByText(/configured share <0.1%/)).toBeVisible(); + expect(screen.getByText(/configured share 99.9%/)).toBeVisible(); + + for (const value of ["", "1.5", "1001", "0", "-1"]) { + fireEvent.change(screen.getByLabelText("Weight for member 2"), { + target: { value }, + }); + expect( + screen.getAllByText( + /configured share unavailable until enabled weights are valid/, + ), + ).toHaveLength(2); + expect( + screen.queryByText(/configured share [0-9]|NaN%|Infinity%/), + ).not.toBeInTheDocument(); + await waitFor(() => expect(saveButton()).toBeDisabled()); + } + fireEvent.change(screen.getByLabelText("Weight for member 2"), { + target: { value: "3" }, + }); + expect(screen.getByText(/configured share 75%/)).toBeVisible(); + await user.click( + screen.getByRole("button", { + name: "Move connection 2 earlier in the cycle", + }), + ); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ + expected_revision: 17, + members: [ + expect.objectContaining({ user_service_id: "second-id", weight: 3 }), + expect.objectContaining({ user_service_id: "member-id", weight: 1 }), + ], + }), + ); + }); + it("invalidates only the affected weighted priority tier and reports invalid priorities neutrally", async () => { + const first = { ...pool.members[0]!, weight: 3, priority: 0 }; + const second = { + ...pool.members[0]!, + user_service_id: "second-id", + weight: 1, + priority: 0, + }; + const third = { + ...pool.members[0]!, + user_service_id: "third-id", + weight: 1001, + priority: 1, + }; + const rows = [ + candidate, + { + ...candidate, + user_service_id: "second-id", + name: "Second connection", + slug: "second", + }, + { + ...candidate, + user_service_id: "third-id", + name: "Third connection", + slug: "third", + }, + ]; + mocks.candidates.mockReturnValue(page(rows)); + mocks.health.mockReturnValue({ + data: { + candidates: rows, + operation_checked: false, + method: null, + path: null, + }, + }); + const user = userEvent.setup(); + render( + , + ); + expect(screen.getByText(/configured tier share 75%/)).toBeVisible(); + expect(screen.getByText(/configured tier share 25%/)).toBeVisible(); + expect( + screen.getByText( + /configured share unavailable until enabled weights in this tier are valid/, + ), + ).toBeVisible(); + fireEvent.change(screen.getByLabelText("Weight for member 2"), { + target: { value: "1.5" }, + }); + expect( + screen.getAllByText( + /configured share unavailable until enabled weights in this tier are valid/, + ), + ).toHaveLength(3); + expect(screen.queryByText(/configured tier share/)).not.toBeInTheDocument(); + await user.click(screen.getByRole("switch", { name: "Member 2 enabled" })); + expect(screen.getByText(/configured tier share 100%/)).toBeVisible(); + expect( + screen.getByText(/disabled · excluded from tier share/), + ).toBeVisible(); + await user.clear(screen.getByLabelText("Priority for member 3")); + expect( + screen.getByText( + /Priority tier needs a valid number · configured share unavailable/, + ), + ).toBeVisible(); + expect( + screen.queryByText(/Priority tier NaN|NaN%|Infinity%/), + ).not.toBeInTheDocument(); + await waitFor(() => expect(saveButton()).toBeDisabled()); + }); +}); + +it.each(["Round robin", "Automatic fallback"])( + "can save %s after hiding an invalid weighted draft", + async (routing) => { + const user = userEvent.setup(); + render( + , + ); + await user.clear(screen.getByLabelText("Weight for member 1")); + await waitFor(() => expect(saveButton()).toBeDisabled()); + await user.click(screen.getByRole("combobox", { name: "Routing" })); + await user.click(screen.getByRole("option", { name: routing })); + expect( + screen.queryByLabelText("Weight for member 1"), + ).not.toBeInTheDocument(); + await waitFor(() => expect(saveButton()).toBeEnabled()); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ + strategy: routing === "Round robin" ? "round_robin" : "priority", + members: [expect.objectContaining({ weight: 1 })], + }), + ); + }, +); + +it("can save Take turns after hiding an invalid tier weight", async () => { + const user = userEvent.setup(); + render( + , + ); + await user.clear(screen.getByLabelText("Weight for member 1")); + await user.click( + screen.getByRole("combobox", { + name: "Connections with the same priority", + }), + ); + await user.click(screen.getByRole("option", { name: "Take turns" })); + await waitFor(() => expect(saveButton()).toBeEnabled()); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ + tier_balance: "round_robin", + members: [expect.objectContaining({ weight: 1 })], + }), + ); +}); + +it("retains priorities, models, tier balancing and retries across a routing round trip", async () => { + const { defaultFailoverPolicy } = await import("@/schemas/pools"); + const user = userEvent.setup(); + const original = { + ...pool, + member_contract: "ai_chat" as const, + tier_balance: "weighted" as const, + failover: { ...defaultFailoverPolicy, max_attempts: 4 }, + members: [{ ...pool.members[0]!, priority: 10, model: "chat-model" }], + }; + render(); + for (const routing of ["Round robin", "Automatic fallback"]) { + await user.click(screen.getByRole("combobox", { name: "Routing" })); + await user.click(screen.getByRole("option", { name: routing })); + } + expect(screen.getByLabelText("Priority for member 1")).toHaveValue(10); + expect(screen.getByLabelText("Model for member 1")).toHaveValue("chat-model"); + expect( + screen.getByRole("combobox", { + name: "Connections with the same priority", + }), + ).toHaveTextContent("Share by weight"); + await user.type(screen.getByLabelText("Name"), " changed"); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ + members: original.members, + failover: original.failover, + tier_balance: "weighted", + member_contract: "ai_chat", + }), + ); +}); + +it.each(["weighted", "round_robin"] as const)( + "moves connections within their %s priority tier without changing settings", + async (tierBalance) => { + const user = userEvent.setup(); + const first = { ...pool.members[0]!, weight: 2, priority: 0 }; + const backup = { ...first, user_service_id: "backup", priority: 10 }; + const second = { ...first, user_service_id: "second", weight: 1 }; + render( + , + ); + expect( + screen.getByRole("button", { + name: "Move connection 1 earlier within priority 0", + }), + ).toBeDisabled(); + await user.click( + screen.getByRole("button", { + name: "Move connection 2 earlier within priority 0", + }), + ); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ members: [second, backup, first] }), + ); + }, +); + +it("reloads a conflicted draft and saves using the latest revision", async () => { + const { ApiError } = await import("@/lib/api-client"); + const user = userEvent.setup(); + mocks.update.mockRejectedValueOnce( + new ApiError(409, { + error: "conflict", + error_code: 1009, + message: "Changed elsewhere", + }), + ); + mocks.reload.mockResolvedValue({ + ...pool, + name: "Latest pool", + config_revision: 18, + }); + render(); + await user.type(screen.getByLabelText("Name"), " stale"); + await user.click(saveButton()); + await user.click( + await screen.findByRole("button", { name: "Reload latest" }), + ); + await waitFor(() => + expect(screen.getByLabelText("Name")).toHaveValue("Latest pool"), + ); + await user.type(screen.getByLabelText("Name"), " edited"); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenLastCalledWith( + expect.objectContaining({ + name: "Latest pool edited", + expected_revision: 18, + }), + ); +}); diff --git a/frontend/src/components/dashboard/service-pools-tab.test.tsx b/frontend/src/components/dashboard/service-pools-tab.test.tsx index 15338de58..001c6b387 100644 --- a/frontend/src/components/dashboard/service-pools-tab.test.tsx +++ b/frontend/src/components/dashboard/service-pools-tab.test.tsx @@ -9,6 +9,7 @@ import { import type { PoolCandidate, ServicePool } from "@/schemas/pools"; const mocks = vi.hoisted(() => ({ update: vi.fn(), + reload: vi.fn(), create: vi.fn(), candidates: vi.fn(), health: vi.fn(), @@ -18,6 +19,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock("@/hooks/use-pools", () => ({ useUpdateServicePool: () => ({ mutateAsync: mocks.update, isPending: false }), + useReloadServicePool: () => ({ mutateAsync: mocks.reload, isPending: false }), useCreateServicePool: () => ({ mutateAsync: mocks.create, isPending: false }), usePoolCandidates: mocks.candidates, useDeleteServicePool: () => ({ isPending: false }), diff --git a/frontend/src/hooks/use-pools.test.tsx b/frontend/src/hooks/use-pools.test.tsx index 5b898c49d..9da89bce4 100644 --- a/frontend/src/hooks/use-pools.test.tsx +++ b/frontend/src/hooks/use-pools.test.tsx @@ -3,6 +3,7 @@ import { act, renderHook, waitFor } from "@testing-library/react"; import type { PropsWithChildren } from "react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { + useServicePools, usePoolCandidates, usePoolHealth, useResetPoolHealth, @@ -15,12 +16,16 @@ const api = vi.hoisted(() => ({ put: vi.fn(), delete: vi.fn(), })); -vi.mock("@/lib/api-client", () => ({ api })); +vi.mock("@/lib/api-client", async (importOriginal) => ({ + ...(await importOriginal()), + api, + apiClient: api.get, +})); -function wrapperFactory() { +function wrapperFactory({ staleTime = 0, gcTime = 0 } = {}) { const client = new QueryClient({ defaultOptions: { - queries: { retry: false, gcTime: 0 }, + queries: { retry: false, gcTime, staleTime }, mutations: { retry: false }, }, }); @@ -31,6 +36,107 @@ function wrapperFactory() { beforeEach(() => vi.resetAllMocks()); +it("finishes pending pagination then checks every loaded page against the latest selection", async () => { + let release: (() => void) | undefined; + let pageSignal: AbortSignal | undefined; + api.get.mockImplementation( + async (path: string, { signal }: { signal: AbortSignal }) => { + const query = new URL(path, "https://nyxid.invalid").searchParams; + const peer = query.get("peer_ids"); + if (query.has("after") && peer === "") { + pageSignal = signal; + await new Promise((resolve) => { + release = resolve; + }); + } + return { + candidates: [ + { + user_service_id: query.has("after") ? "second" : "first", + eligible: peer !== "selected", + }, + ], + next_cursor: query.has("after") ? null : "100", + has_more: !query.has("after"), + }; + }, + ); + const { result, rerender } = renderHook( + ({ peers }) => usePoolCandidates({ peerIds: peers }), + { wrapper: wrapperFactory(), initialProps: { peers: [] as string[] } }, + ); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + let pending: Promise; + act(() => { + pending = result.current.fetchNextPage(); + }); + await waitFor(() => expect(release).toBeDefined()); + rerender({ peers: ["selected"] }); + expect(result.current.isCheckingCompatibility).toBe(true); + expect(pageSignal?.aborted).toBe(false); + await act(async () => { + release!(); + await pending; + }); + await waitFor(() => + expect(result.current.isCheckingCompatibility).toBe(false), + ); + expect(result.current.data?.pages).toHaveLength(2); + expect( + result.current.data?.pages + .flatMap((page) => page.candidates) + .every((row) => !row.eligible), + ).toBe(true); + expect(api.get).toHaveBeenCalledTimes(4); +}); + +it("marks cached rows busy until the current selection has been checked and stops after an error", async () => { + let release: (() => void) | undefined; + api.get.mockImplementation(async (path: string) => { + const query = new URL(path, "https://nyxid.invalid").searchParams; + if (!query.has("search") && query.get("peer_ids") === "selected") { + await new Promise((resolve) => { + release = resolve; + }); + throw new Error("Temporary inventory failure"); + } + return { + candidates: [{ user_service_id: "first", eligible: true }], + has_more: false, + next_cursor: null, + }; + }); + const { result, rerender } = renderHook( + ({ search, peers }) => usePoolCandidates({ search, peerIds: peers }), + { + wrapper: wrapperFactory({ staleTime: 60000, gcTime: 300000 }), + initialProps: { search: "", peers: [] as string[] }, + }, + ); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + rerender({ search: "backup", peers: ["selected"] }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + rerender({ search: "", peers: ["selected"] }); + expect(result.current.isCheckingCompatibility).toBe(true); + await waitFor(() => expect(release).toBeDefined()); + await act(async () => { + release!(); + }); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.isCheckingCompatibility).toBe(true); + expect(api.get).toHaveBeenCalledTimes(3); + api.get.mockResolvedValue({ + candidates: [{ user_service_id: "first", eligible: false }], + has_more: false, + next_cursor: null, + }); + await act(() => result.current.refetch()); + await waitFor(() => + expect(result.current.isCheckingCompatibility).toBe(false), + ); + expect(result.current.data?.pages[0]?.candidates[0]?.eligible).toBe(false); +}); + describe("pool management requests", () => { it("sends configuration and members in one PUT and preserves revision and explicit clears", async () => { api.put.mockResolvedValue({ id: "pool-id", config_revision: 8 }); @@ -247,3 +353,140 @@ describe("pool management requests", () => { expect(healthUrl.searchParams.get("path")).toBe("chat/completions"); }); }); + +it("keeps every loaded inventory page while draft compatibility refreshes", async () => { + let release: (() => void) | undefined; + api.get.mockImplementation(async (path: string) => { + const query = new URL(path, "https://nyxid.invalid").searchParams; + const peers = query.get("peer_ids"); + if (peers === "selected" && !query.has("after")) + await new Promise((resolve) => { + release = resolve; + }); + return { + candidates: [ + { + user_service_id: query.has("after") ? "second" : "first", + reason: + peers === "selected" ? "compatibility_declaration_required" : null, + }, + ], + next_cursor: query.has("after") ? null : "100", + has_more: !query.has("after"), + }; + }); + const { result, rerender } = renderHook( + ({ peers }) => usePoolCandidates({ peerIds: peers, checkOperation: false }), + { + wrapper: wrapperFactory(), + initialProps: { peers: [] as string[] }, + }, + ); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + expect(result.current.data?.pages).toHaveLength(1); + await act(() => result.current.fetchNextPage()); + await waitFor(() => expect(result.current.data?.pages).toHaveLength(2)); + rerender({ peers: ["selected"] }); + await waitFor(() => expect(release).toBeDefined()); + expect(result.current.data?.pages).toHaveLength(2); + expect(result.current.isLoading).toBe(false); + await act(async () => { + release!(); + }); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + expect(result.current.data?.pages).toHaveLength(2); + expect(result.current.data?.pages[1]?.candidates[0]?.reason).toBe( + "compatibility_declaration_required", + ); +}); + +it("refreshes the list after a revision conflict so reopening uses the latest pool", async () => { + const { ApiError } = await import("@/lib/api-client"); + api.get + .mockResolvedValueOnce({ pools: [{ id: "pool", config_revision: 1 }] }) + .mockResolvedValue({ pools: [{ id: "pool", config_revision: 2 }] }); + api.put.mockRejectedValue( + new ApiError(409, { + error: "conflict", + error_code: 1009, + message: "Changed elsewhere", + }), + ); + const { result } = renderHook( + () => ({ pools: useServicePools(), update: useUpdateServicePool() }), + { + wrapper: wrapperFactory(), + }, + ); + await waitFor(() => + expect(result.current.pools.data?.[0]?.config_revision).toBe(1), + ); + await act(async () => { + await expect( + result.current.update.mutateAsync({ + poolId: "pool", + name: "stale", + expected_revision: 1, + }), + ).rejects.toMatchObject({ status: 409 }); + }); + await waitFor(() => + expect(result.current.pools.data?.[0]?.config_revision).toBe(2), + ); + expect(api.put).toHaveBeenCalledTimes(1); +}); + +it.each(["peerIds", "declaredPeerIds"] as const)( + "rechecks a cached search after %s changes under production cache settings", + async (field) => { + api.get.mockImplementation(async (path: string) => { + const query = new URL(path, "https://nyxid.invalid").searchParams; + const changed = + query.get(field === "peerIds" ? "peer_ids" : "declared_peer_ids") === + "B"; + return { + candidates: [{ user_service_id: "candidate", eligible: !changed }], + has_more: false, + next_cursor: null, + }; + }); + const { result, rerender } = renderHook( + ({ search, ids }) => usePoolCandidates({ search, [field]: ids }), + { + wrapper: wrapperFactory({ staleTime: 60000, gcTime: 300000 }), + initialProps: { search: "", ids: [] as string[] }, + }, + ); + await waitFor(() => + expect(result.current.data?.pages[0]?.candidates[0]?.eligible).toBe(true), + ); + rerender({ search: "backup", ids: [] }); + await waitFor(() => expect(api.get).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(result.current.data?.pages[0]?.candidates[0]?.eligible).toBe(true), + ); + rerender({ search: "backup", ids: ["B"] }); + await waitFor(() => + expect(result.current.data?.pages[0]?.candidates[0]?.eligible).toBe( + false, + ), + ); + rerender({ search: "", ids: ["B"] }); + await waitFor(() => + expect(result.current.data?.pages[0]?.candidates[0]?.eligible).toBe( + false, + ), + ); + expect(api.get).toHaveBeenCalledTimes(4); + const request = new URL( + api.get.mock.calls.at(-1)![0], + "https://nyxid.invalid", + ); + expect(request.searchParams.get("search")).toBeNull(); + expect( + request.searchParams.get( + field === "peerIds" ? "peer_ids" : "declared_peer_ids", + ), + ).toBe("B"); + }, +); diff --git a/frontend/src/hooks/use-pools.ts b/frontend/src/hooks/use-pools.ts index a4389fa70..f8cde89ae 100644 --- a/frontend/src/hooks/use-pools.ts +++ b/frontend/src/hooks/use-pools.ts @@ -1,10 +1,12 @@ import { + hashKey, useInfiniteQuery, useMutation, useQuery, useQueryClient, } from "@tanstack/react-query"; -import { api } from "@/lib/api-client"; +import { useEffect, useRef } from "react"; +import { api, apiClient, ApiError } from "@/lib/api-client"; import type { PoolCandidatesResponse, CreateServicePoolInput, @@ -82,6 +84,23 @@ export function useUpdateServicePool() { }, onSuccess: (_data, variables) => invalidatePools(queryClient, variables.poolId), + onError: (error, variables) => { + if (error instanceof ApiError && error.status === 409) + invalidatePools(queryClient, variables.poolId); + }, + }); +} + +export function useReloadServicePool() { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: (poolId: string) => + queryClient.fetchQuery({ + queryKey: [...SERVICE_POOLS_KEY, poolId], + staleTime: 0, + queryFn: () => + api.get(`/service-pools/${encodeURIComponent(poolId)}`), + }), }); } @@ -192,17 +211,62 @@ export function usePoolCandidates( options: PoolInspectionOptions, enabled = true, ) { - return useInfiniteQuery({ - queryKey: [...SERVICE_POOLS_KEY, "candidates", options], + const identity = options.selectedOnly + ? options + : { + ...options, + peerIds: undefined, + declaredPeerIds: undefined, + }; + const queryKey = [...SERVICE_POOLS_KEY, "candidates", identity]; + const scope = hashKey(queryKey); + const peers = JSON.stringify([options.peerIds, options.declaredPeerIds]); + const previous = useRef({ scope, peers }); + const result = useInfiniteQuery({ + queryKey, enabled, + // Cached searches may have been checked against a different draft selection. + staleTime: 0, initialPageParam: undefined as string | undefined, - queryFn: ({ pageParam }) => - api.get( + queryFn: async ({ pageParam, signal }) => ({ + ...(await apiClient( inspectionPath(options, false, pageParam), - ), + { signal }, + )), + inspectionPeers: peers, + }), getNextPageParam: (page) => page.has_more ? (page.next_cursor ?? undefined) : undefined, }); + const { refetch, isFetching, isError } = result; + const isCheckingCompatibility = Boolean( + result.data?.pages.some((page) => page.inspectionPeers !== peers), + ); + useEffect(() => { + const changed = + previous.current.scope === scope && previous.current.peers !== peers; + previous.current = { scope, peers }; + // Finish pending pagination before refreshing every loaded page for the draft. + if ( + enabled && + !options.selectedOnly && + !isFetching && + isCheckingCompatibility && + (changed || !isError) + ) { + void refetch({ cancelRefetch: false }); + } + }, [ + scope, + peers, + enabled, + options.selectedOnly, + refetch, + isFetching, + isError, + isCheckingCompatibility, + ]); + return { ...result, isCheckingCompatibility }; } export function usePoolHealth(options: PoolInspectionOptions) { return useQuery({ diff --git a/frontend/src/schemas/pools.test.ts b/frontend/src/schemas/pools.test.ts new file mode 100644 index 000000000..614d44e8b --- /dev/null +++ b/frontend/src/schemas/pools.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; +import { createServicePoolSchema, updateServicePoolSchema } from "./pools"; + +const input = { + name: "Pool", + slug: "pool", + strategy: "priority" as const, + member_contract: "ai_chat" as const, + members: [ + { user_service_id: "member", weight: 1, enabled: true, model: "model" }, + ], +}; + +describe("pool Unicode limits", () => { + it.each(["a", "服", "🪐"])( + "matches server character boundaries for %s", + (character) => { + const boundary = { + ...input, + name: character.repeat(128), + description: character.repeat(1024), + members: [{ ...input.members[0]!, model: character.repeat(256) }], + }; + for (const schema of [createServicePoolSchema, updateServicePoolSchema]) { + expect(schema.safeParse(boundary).success).toBe(true); + expect( + schema.safeParse({ ...boundary, name: character.repeat(129) }) + .success, + ).toBe(false); + expect( + schema.safeParse({ ...boundary, description: character.repeat(1025) }) + .success, + ).toBe(false); + expect( + schema.safeParse({ + ...boundary, + members: [{ ...input.members[0]!, model: character.repeat(257) }], + }).success, + ).toBe(false); + } + }, + ); +}); diff --git a/frontend/src/schemas/pools.ts b/frontend/src/schemas/pools.ts index c19f92ef5..b73fedfe2 100644 --- a/frontend/src/schemas/pools.ts +++ b/frontend/src/schemas/pools.ts @@ -74,12 +74,31 @@ const slugSchema = z /^[a-z0-9]+(?:-[a-z0-9]+)*$/, "Use lowercase letters, numbers, and single hyphens", ); +const weightMessage = "Enter a whole-number weight from 1 to 1000"; +const descriptionSchema = z + .string() + .refine( + (value) => [...value].length <= 1024, + "Description must be 1024 characters or fewer", + ); export const poolMemberSchema = z.object({ user_service_id: z.string().min(1, "Select a service"), - weight: z.number().int().min(1).max(1000), + weight: z + .number({ error: weightMessage }) + .int(weightMessage) + .min(1, weightMessage) + .max(1000, weightMessage), enabled: z.boolean(), priority: z.number().int().min(0).max(4294967295).optional(), - model: z.string().trim().max(256).nullable().optional(), + model: z + .string() + .trim() + .refine( + (value) => [...value].length <= 256, + "Model must be 256 characters or fewer", + ) + .nullable() + .optional(), same_api_compatible: z.boolean().optional(), }); export const servicePoolSchema = z.object({ @@ -104,8 +123,15 @@ export const servicePoolListResponseSchema = z.object({ }); const poolInputSchema = z.object({ slug: slugSchema, - name: z.string().trim().min(1, "Name is required").max(128), - description: z.string().max(1024).optional(), + name: z + .string() + .trim() + .min(1, "Name is required") + .refine( + (value) => [...value].length <= 128, + "Name must be 128 characters or fewer", + ), + description: descriptionSchema.optional(), strategy: poolStrategySchema, tier_balance: z.enum(["round_robin", "weighted"]).optional(), member_contract: poolContractSchema.optional(), @@ -142,7 +168,7 @@ export const updateServicePoolSchema = poolInputSchema .partial() .extend({ expected_revision: z.number().int().optional(), - description: z.string().max(1024).nullable().optional(), + description: descriptionSchema.nullable().optional(), }); export const setPoolMembersSchema = z.object({ members: z.array(poolMemberSchema).max(50), @@ -165,6 +191,9 @@ export interface PoolCandidate { credential_binding: string; protocol: string | null; catalog_service_id: string | null; + /** Original catalog metadata used to group connected accounts in the picker. */ + group_name?: string | null; + group_slug?: string | null; requires_compatibility_declaration: boolean; cooldown_until: string | null; consecutive_failures: number;