From 7011395fcf06734110dee5d86a06325fa0665e08 Mon Sep 17 00:00:00 2001
From: prxnvt <53838599+prxnvt@users.noreply.github.com>
Date: Wed, 21 Oct 2026 15:21:33 -0400
Subject: [PATCH] Engine: a macro renamed on a name clash keeps its name within
 the length limit

When a file held two macros with the same name, the second was renamed
"name (2)" without room for the suffix. Two macros with the same
255-character name both opened, but the second name grew to 259
characters, was saved that way, and the next open dropped that macro as
over the limits. unique_name now shortens the name to make room for the
suffix, and load checks the macro again after renaming it.
---
 crates/engine/src/macros/mod.rs   | 27 +++++++++++++++++++--------
 crates/engine/src/macros/tests.rs | 28 ++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs
index 6b3ccdbd8..68db8e249 100644
--- a/crates/engine/src/macros/mod.rs
+++ b/crates/engine/src/macros/mod.rs
@@ -128,12 +128,14 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
             bad += 1;
             continue;
         }
-        match m.check_limits().and_then(|()| check_size(&m)) {
-            Ok(()) => {
-                let mut m = m;
-                m.name = unique_name(wb, m.name.trim());
-                wb.macros.push(m);
-            }
+        let mut m = m;
+        let checked = m.check_limits().and_then(|()| {
+            m.name = unique_name(wb, m.name.trim());
+            // The macro as it is kept, renamed, must still pass, or the next save would lose it.
+            m.check_limits().and_then(|()| check_size(&m))
+        });
+        match checked {
+            Ok(()) => wb.macros.push(m),
             Err(_) => bad += 1,
         }
     }
@@ -150,13 +152,22 @@ pub fn load(wb: &mut Workbook) -> Vec<String> {
     warnings
 }
 
-/// `name`, or `name (2)`, `name (3)`… if a macro already has it.
+/// `name`, or `name (2)`, `name (3)`… if a macro already has it. A long name is shortened to make
+/// room for the suffix, so the result stays within [`gridcraft_model::MAX_MACRO_NAME`] characters.
 pub fn unique_name(wb: &Workbook, name: &str) -> String {
     let name = if name.is_empty() { "Macro" } else { name };
     if find(wb, name).is_none() {
         return name.to_string();
     }
-    (2..).map(|n| format!("{name} ({n})")).find(|n| find(wb, n).is_none()).unwrap_or_else(|| name.to_string())
+    (2..)
+        .map(|n| {
+            let suffix = format!(" ({n})");
+            let keep = gridcraft_model::MAX_MACRO_NAME.saturating_sub(suffix.chars().count());
+            let base: String = name.chars().take(keep).collect();
+            format!("{}{suffix}", base.trim_end())
+        })
+        .find(|n| find(wb, n).is_none())
+        .unwrap_or_else(|| name.to_string())
 }
 
 #[cfg(test)]
diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs
index fea61f6c3..e67479eab 100644
--- a/crates/engine/src/macros/tests.rs
+++ b/crates/engine/src/macros/tests.rs
@@ -190,6 +190,13 @@ fn names() {
     assert_eq!(unique_name(&wb, "macro1"), "macro1 (2)");
     assert_eq!(unique_name(&wb, "Other"), "Other");
     assert_eq!(unique_name(&wb, ""), "Macro");
+    // A long name is shortened to make room for the suffix.
+    let long = "y".repeat(MAX_MACRO_NAME);
+    wb.macros.push(Macro { name: long.clone(), ..Macro::default() });
+    let renamed = unique_name(&wb, &long);
+    assert_eq!(renamed, format!("{} (2)", "y".repeat(MAX_MACRO_NAME - 4)));
+    wb.macros.push(Macro { name: renamed, ..Macro::default() });
+    assert_eq!(unique_name(&wb, &long).chars().count(), MAX_MACRO_NAME);
     assert_eq!(find(&wb, " MACRO1 "), Some(0));
     assert_eq!(find(&wb, "Macro2"), None);
 }
@@ -206,3 +213,24 @@ fn json_field_macros_get_the_name_check_too() {
     assert_eq!(names, ["Fine"]);
     assert!(warnings.len() == 1 && warnings[0].starts_with("1 macro "), "{warnings:?}");
 }
+
+#[test]
+fn duplicate_names_at_the_length_limit_survive_save_and_reopen() {
+    // Two macros with the same 255-character name: the second is renamed within the limit, so a
+    // save and a second open keep both.
+    let long = "é".repeat(MAX_MACRO_NAME);
+    for file in ["dup.json", "dup.xlsx"] {
+        let mut wb = Workbook::new();
+        wb.macros.push(Macro { name: long.clone(), ..Macro::default() });
+        wb.macros.push(Macro { name: long.clone(), description: "second".into(), ..Macro::default() });
+        let (first, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&wb, file).unwrap()).unwrap();
+        assert!(warnings.is_empty(), "{file}: {warnings:?}");
+        assert_eq!(first.macros.len(), 2, "{file}");
+        let renamed = &first.macros[1].name;
+        assert_eq!(renamed.chars().count(), MAX_MACRO_NAME, "{file}");
+        assert!(renamed.ends_with(" (2)") && renamed.starts_with("éé"), "{file}: {renamed}");
+        let (again, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&first, file).unwrap()).unwrap();
+        assert!(warnings.is_empty(), "{file}: {warnings:?}");
+        assert_eq!(again.macros, first.macros, "{file}");
+    }
+}