diff --git a/src-tauri/src/data/bridge.rs b/src-tauri/src/data/bridge.rs index 7d8a0f5..bc651e3 100644 --- a/src-tauri/src/data/bridge.rs +++ b/src-tauri/src/data/bridge.rs @@ -77,36 +77,65 @@ pub enum MappingKey { MrAuthored { project: String, iid: i64 }, } +/// Maximum length for entity titles in bead titles (to keep beads scannable) +const MAX_TITLE_LENGTH: usize = 60; + +/// Truncate a string to max_len characters, appending "..." if truncated. +/// Handles Unicode correctly by counting grapheme clusters. +fn truncate_title(s: &str, max_len: usize) -> String { + if s.chars().count() <= max_len { + s.to_string() + } else { + let truncated: String = s.chars().take(max_len.saturating_sub(3)).collect(); + format!("{}...", truncated.trim_end()) + } +} + impl MappingKey { - /// Serialize to string key format + /// Serialize to string key format. + /// + /// Keys are designed to be: + /// - Stable across project renames (using project path as lore doesn't expose project_id yet) + /// - Safe for JSON keys and filesystem paths (no spaces, forward slashes escaped) + /// - Unique within an MC instance pub fn to_key_string(&self) -> String { match self { MappingKey::MrReview { project, iid } => { - format!("mr_review:{}:{}", project, iid) + format!("mr_review:{}:{}", Self::escape_project(project), iid) } MappingKey::Issue { project, iid } => { - format!("issue:{}:{}", project, iid) + format!("issue:{}:{}", Self::escape_project(project), iid) } MappingKey::MrAuthored { project, iid } => { - format!("mr_authored:{}:{}", project, iid) + format!("mr_authored:{}:{}", Self::escape_project(project), iid) } } } - /// Build bead title from this key's event data + /// Build bead title from this key's event data. + /// + /// Titles are formatted as "{prefix} {entity_title}" with truncation + /// to keep them scannable in the UI. pub fn to_bead_title(&self, entity_title: &str) -> String { + let truncated = truncate_title(entity_title, MAX_TITLE_LENGTH); match self { MappingKey::MrReview { iid, .. } => { - format!("Review MR !{}: {}", iid, entity_title) + format!("Review MR !{}: {}", iid, truncated) } MappingKey::Issue { iid, .. } => { - format!("Issue #{}: {}", iid, entity_title) + format!("Issue #{}: {}", iid, truncated) } MappingKey::MrAuthored { iid, .. } => { - format!("Your MR !{}: {}", iid, entity_title) + format!("Your MR !{}: {}", iid, truncated) } } } + + /// Escape project path for use in mapping keys. + /// Replaces / with :: to make keys filesystem-safe. + fn escape_project(project: &str) -> String { + project.replace('/', "::") + } } /// Result of a sync operation @@ -683,19 +712,47 @@ mod tests { project: "group/repo".to_string(), iid: 847, }; - assert_eq!(key.to_key_string(), "mr_review:group/repo:847"); + // Project path / is escaped to :: for filesystem safety + assert_eq!(key.to_key_string(), "mr_review:group::repo:847"); let key = MappingKey::Issue { project: "group/repo".to_string(), iid: 42, }; - assert_eq!(key.to_key_string(), "issue:group/repo:42"); + assert_eq!(key.to_key_string(), "issue:group::repo:42"); let key = MappingKey::MrAuthored { project: "group/repo".to_string(), iid: 100, }; - assert_eq!(key.to_key_string(), "mr_authored:group/repo:100"); + assert_eq!(key.to_key_string(), "mr_authored:group::repo:100"); + } + + #[test] + fn test_mapping_key_escapes_nested_groups() { + // GitLab supports deeply nested groups like org/team/sub/repo + let key = MappingKey::Issue { + project: "org/team/sub/repo".to_string(), + iid: 42, + }; + assert_eq!(key.to_key_string(), "issue:org::team::sub::repo:42"); + } + + #[test] + fn test_mapping_key_safe_for_filesystem() { + let key = MappingKey::MrReview { + project: "group/repo".to_string(), + iid: 847, + }; + let key_str = key.to_key_string(); + + // Keys should not contain characters that are problematic for: + // - JSON object keys (no quotes, backslashes) + // - Filesystem paths (no forward slashes, colons are acceptable on Unix) + assert!(!key_str.contains('/'), "Key should not contain forward slash"); + assert!(!key_str.contains(' '), "Key should not contain spaces"); + assert!(!key_str.contains('"'), "Key should not contain quotes"); + assert!(!key_str.contains('\\'), "Key should not contain backslashes"); } #[test] @@ -728,6 +785,66 @@ mod tests { ); } + #[test] + fn test_bead_title_truncates_long_titles() { + let key = MappingKey::MrReview { + project: "g/p".to_string(), + iid: 847, + }; + + let long_title = "Fix authentication token refresh logic that was causing intermittent failures in production"; + let title = key.to_bead_title(long_title); + + // Title should be truncated with ellipsis + assert!(title.ends_with("..."), "Long title should end with ellipsis"); + // The entity_title portion should be max 60 chars + // "Review MR !847: " is 16 chars, so total should be under 16 + 60 = 76 + assert!(title.len() <= 80, "Title should be reasonably short: {}", title); + } + + #[test] + fn test_bead_title_preserves_short_titles() { + let key = MappingKey::Issue { + project: "g/p".to_string(), + iid: 42, + }; + + let short_title = "Quick fix"; + let title = key.to_bead_title(short_title); + + assert!(!title.ends_with("..."), "Short title should not be truncated"); + assert_eq!(title, "Issue #42: Quick fix"); + } + + #[test] + fn test_truncate_title_exactly_at_limit() { + // 60 char title should not be truncated + let title_60 = "A".repeat(60); + let truncated = truncate_title(&title_60, 60); + assert_eq!(truncated.len(), 60); + assert!(!truncated.ends_with("...")); + } + + #[test] + fn test_truncate_title_just_over_limit() { + // 61 char title should be truncated + let title_61 = "A".repeat(61); + let truncated = truncate_title(&title_61, 60); + assert!(truncated.ends_with("...")); + assert!(truncated.len() <= 60); + } + + #[test] + fn test_truncate_title_handles_unicode() { + // Unicode characters should be counted correctly, not by bytes + let emoji_title = "Fix 🔥 auth bug with 中文 characters that is very long indeed"; + let truncated = truncate_title(emoji_title, 30); + + // Should truncate by character count, not bytes + assert!(truncated.chars().count() <= 30); + assert!(truncated.ends_with("...")); + } + // -- Map persistence tests -- #[test] @@ -767,7 +884,7 @@ mod tests { let mut map = GitLabBeadMap::default(); map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-abc".to_string()), created_at: "2026-02-25T10:00:00Z".to_string(), @@ -781,9 +898,9 @@ mod tests { let loaded = bridge.load_map().unwrap(); assert_eq!(loaded.mappings.len(), 1); - assert!(loaded.mappings.contains_key("issue:g/p:42")); + assert!(loaded.mappings.contains_key("issue:g::p:42")); assert_eq!( - loaded.mappings["issue:g/p:42"].bead_id, + loaded.mappings["issue:g::p:42"].bead_id, Some("bd-abc".to_string()) ); } @@ -823,7 +940,7 @@ mod tests { assert!(created.unwrap()); assert_eq!(map.mappings.len(), 1); - let entry = &map.mappings["issue:g/p:42"]; + let entry = &map.mappings["issue:g::p:42"]; assert_eq!(entry.bead_id, Some("bd-new".to_string())); assert!(!entry.pending); } @@ -836,7 +953,7 @@ mod tests { // Pre-populate map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-existing".to_string()), created_at: "2026-02-25T10:00:00Z".to_string(), @@ -901,7 +1018,7 @@ mod tests { // Simulate crashed state: pending=true, bead_id=None map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: None, created_at: "2026-02-25T10:00:00Z".to_string(), @@ -916,7 +1033,7 @@ mod tests { assert_eq!(recovered, 1); assert!(errors.is_empty()); - let entry = &map.mappings["issue:g/p:42"]; + let entry = &map.mappings["issue:g::p:42"]; assert_eq!(entry.bead_id, Some("bd-recovered".to_string())); assert!(!entry.pending); } @@ -929,7 +1046,7 @@ mod tests { // Simulate: bead was created but pending flag not cleared map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-exists".to_string()), created_at: "2026-02-25T10:00:00Z".to_string(), @@ -944,7 +1061,7 @@ mod tests { assert_eq!(recovered, 1); assert!(errors.is_empty()); - assert!(!map.mappings["issue:g/p:42"].pending); + assert!(!map.mappings["issue:g::p:42"].pending); } // -- incremental_sync tests -- @@ -988,7 +1105,7 @@ mod tests { assert_eq!(result.created, 1); assert_eq!(result.skipped, 0); - assert!(map.mappings.contains_key("issue:g/p:42")); + assert!(map.mappings.contains_key("issue:g::p:42")); assert_eq!( map.cursor.last_check_timestamp, Some("2026-02-25T12:00:00Z".to_string()) @@ -1021,7 +1138,7 @@ mod tests { // Pre-populate so it's a duplicate map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-existing".to_string()), created_at: "2026-02-25T09:00:00Z".to_string(), @@ -1075,8 +1192,8 @@ mod tests { bridge.incremental_sync(&mut map).unwrap(); // Should be classified as mr_review, not mr_authored - assert!(map.mappings.contains_key("mr_review:g/p:100")); - assert!(!map.mappings.contains_key("mr_authored:g/p:100")); + assert!(map.mappings.contains_key("mr_review:g::p:100")); + assert!(!map.mappings.contains_key("mr_authored:g::p:100")); } // -- full_reconciliation tests -- @@ -1097,7 +1214,7 @@ mod tests { // Simulate first strike from previous reconciliation map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-abc".to_string()), created_at: "2026-02-25T09:00:00Z".to_string(), @@ -1110,7 +1227,7 @@ mod tests { let result = bridge.full_reconciliation(&mut map).unwrap(); assert_eq!(result.healed, 1); - assert!(!map.mappings["issue:g/p:42"].suspect_orphan); + assert!(!map.mappings["issue:g::p:42"].suspect_orphan); } #[test] @@ -1125,7 +1242,7 @@ mod tests { let mut map = GitLabBeadMap::default(); map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-abc".to_string()), created_at: "2026-02-25T09:00:00Z".to_string(), @@ -1139,7 +1256,7 @@ mod tests { // First strike: should be marked suspect, NOT closed assert_eq!(result.closed, 0); - assert!(map.mappings["issue:g/p:42"].suspect_orphan); + assert!(map.mappings["issue:g::p:42"].suspect_orphan); } #[test] @@ -1157,7 +1274,7 @@ mod tests { // Already has first strike map.mappings.insert( - "issue:g/p:42".to_string(), + "issue:g::p:42".to_string(), MappingEntry { bead_id: Some("bd-abc".to_string()), created_at: "2026-02-25T09:00:00Z".to_string(), @@ -1171,7 +1288,7 @@ mod tests { // Second strike: should be closed and removed assert_eq!(result.closed, 1); - assert!(!map.mappings.contains_key("issue:g/p:42")); + assert!(!map.mappings.contains_key("issue:g::p:42")); } #[test] @@ -1196,7 +1313,7 @@ mod tests { let result = bridge.full_reconciliation(&mut map).unwrap(); assert_eq!(result.created, 1); - assert!(map.mappings.contains_key("issue:g/p:99")); + assert!(map.mappings.contains_key("issue:g::p:99")); } #[test] @@ -1225,9 +1342,9 @@ mod tests { Bridge::::build_expected_keys(&response); assert_eq!(keys.len(), 3); - assert!(keys.contains_key("issue:g/p:1")); - assert!(keys.contains_key("mr_authored:g/p:10")); - assert!(keys.contains_key("mr_review:g/p:20")); + assert!(keys.contains_key("issue:g::p:1")); + assert!(keys.contains_key("mr_authored:g::p:10")); + assert!(keys.contains_key("mr_review:g::p:20")); } // -- Lock tests -- @@ -1300,7 +1417,7 @@ mod tests { let r2 = bridge2.full_reconciliation(&mut map).unwrap(); assert_eq!(r2.closed, 0); assert_eq!(r2.created, 0); - assert!(!map.mappings["issue:g/p:42"].suspect_orphan); + assert!(!map.mappings["issue:g::p:42"].suspect_orphan); // Phase 3: Issue disappears -- first strike let mut lore3 = MockLoreCli::new(); @@ -1309,7 +1426,7 @@ mod tests { let bridge3 = Bridge::with_data_dir(lore3, MockBeadsCli::new(), dir.path().to_path_buf()); let r3 = bridge3.full_reconciliation(&mut map).unwrap(); assert_eq!(r3.closed, 0); - assert!(map.mappings["issue:g/p:42"].suspect_orphan); + assert!(map.mappings["issue:g::p:42"].suspect_orphan); // Phase 4: Still missing -- second strike, close let mut lore4 = MockLoreCli::new(); @@ -1321,7 +1438,7 @@ mod tests { let bridge4 = Bridge::with_data_dir(lore4, beads4, dir.path().to_path_buf()); let r4 = bridge4.full_reconciliation(&mut map).unwrap(); assert_eq!(r4.closed, 1); - assert!(!map.mappings.contains_key("issue:g/p:42")); + assert!(!map.mappings.contains_key("issue:g::p:42")); } // -- cleanup_tmp_files tests -- diff --git a/src-tauri/src/error.rs b/src-tauri/src/error.rs index d20f766..bde3460 100644 --- a/src-tauri/src/error.rs +++ b/src-tauri/src/error.rs @@ -46,6 +46,7 @@ pub enum McErrorCode { // General errors IoError, InternalError, + InvalidInput, } impl McError { @@ -116,6 +117,15 @@ impl McError { "bv CLI not found -- is beads installed?", ) } + + /// Create an invalid input error + pub fn invalid_input(message: impl Into) -> Self { + Self { + code: McErrorCode::InvalidInput, + message: message.into(), + recoverable: false, + } + } } impl std::fmt::Display for McError {