Components: the component button shows its menu arrow, Create component set is offered only where it works; tests for every refusal (#2095 review)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Model-Effort: Opus 5.5 drove; Opus 5.5 briefed; Sonnet 5.5 wrote at medium

crates/engine/src/cmd/component.rs

@@ -25,7 +25,7 @@ pub(crate) use instance::{swap_at, swap_instance};
 use props::{add_prop, bind_prop, delete_prop, edit_prop, set_props};
 pub(crate) use props::{apply_prop_bindings, check_prop_binding, describe_props};
 use variants::{add_variant, combine, create_set};
-pub use variants::{can_add_variant, can_combine_variants};
+pub use variants::{can_add_variant, can_combine_variants, can_create_set};
 
 /// `component.hideWhenPublishing`: the leading `.` of the names of this file's components and
 /// sets (those `ids` are or are in), put on or taken off.

crates/engine/src/cmd/component/variants.rs

@@ -94,6 +94,32 @@ pub(super) fn combine(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({"id": set}))
 }
 
+/// Why `ids` (distinct) can't each become a component of a new set (`component.createSet`), or
+/// `None` when they can: layers inside an instance, library copies, instances, components and
+/// component sets, and a layer inside another one of `ids`.
+fn create_set_refusal(doc: &Document, ids: &[NodeId]) -> Option<&'static str> {
+    if ids.iter().any(|i| inside_instance(doc, *i)) {
+        Some("layers inside an instance can't become components")
+    } else if ids.iter().any(|i| doc.is_remote(*i)) {
+        Some(crate::library::READ_ONLY)
+    } else if ids.iter().any(|i| {
+        doc.node(*i).is_some_and(|n| matches!(n.kind, NodeKind::Instance { .. } | NodeKind::Component { .. } | NodeKind::ComponentSet { .. }))
+    }) {
+        Some("select layers that aren't components, component sets or instances")
+    } else if roots(doc, ids).len() != ids.len() {
+        Some("a layer can't be in a set with one inside it")
+    } else {
+        None
+    }
+}
+
+/// Whether "Create component set" is on offer for `ids`: at least one layer that
+/// [`create_set_refusal`] lets through.
+pub fn can_create_set(doc: &Document, ids: &[NodeId]) -> bool {
+    let distinct: Vec<NodeId> = ids.iter().copied().collect::<BTreeSet<_>>().into_iter().collect();
+    !distinct.is_empty() && distinct.iter().all(|i| doc.contains(*i)) && create_set_refusal(doc, &distinct).is_none()
+}
+
 /// `component.createSet`: each target becomes a component (as `component.createMultiple` makes
 /// them) and all are wrapped in place in a component set, 20 px around, named as Figma names them:
 /// variants "Property 1=<layer name>" in left-to-right order (a name holding `=` is kept); layers
@@ -104,19 +130,8 @@ pub(super) fn create_set(s: &mut Session, p: &Value) -> Result<Value> {
     let mut seen = BTreeSet::new();
     let ids: Vec<NodeId> = req_targets(cmd, s, p)?.into_iter().filter(|i| seen.insert(*i)).collect();
     let doc = s.doc()?;
-    if ids.iter().any(|i| inside_instance(doc, *i)) {
-        return Err(EngineError::Disabled(cmd.into(), "layers inside an instance can't become components".into()));
-    }
-    if ids.iter().any(|i| doc.is_remote(*i)) {
-        return Err(EngineError::Disabled(cmd.into(), crate::library::READ_ONLY.into()));
-    }
-    if ids.iter().any(|i| {
-        doc.node(*i).is_some_and(|n| matches!(n.kind, NodeKind::Instance { .. } | NodeKind::Component { .. } | NodeKind::ComponentSet { .. }))
-    }) {
-        return Err(EngineError::Disabled(cmd.into(), "select layers that aren't components, component sets or instances".into()));
-    }
-    if roots(doc, &ids).len() != ids.len() {
-        return Err(EngineError::Disabled(cmd.into(), "a layer can't be in a set with one inside it".into()));
+    if let Some(why) = create_set_refusal(doc, &ids) {
+        return Err(EngineError::Disabled(cmd.into(), why.into()));
     }
     let mut made = Vec::new();
     for i in roots(doc, &ids) {

crates/engine/src/components_behaviour_tests.rs

@@ -1153,3 +1153,93 @@ fn create_set_refuses_a_target_inside_another_target_and_leaves_the_document_alo
     assert!(matches!(doc.node(frames[0]).unwrap().kind, NodeKind::Frame));
     assert!(matches!(doc.node(frames[1]).unwrap().kind, NodeKind::Frame));
 }
+
+/// Every node's id, parent, name and kind, to compare a document before and after a refused command.
+fn snapshot(s: &Session) -> Vec<String> {
+    s.doc().unwrap().nodes().map(|n| format!("{:?}|{:?}|{}|{:?}", n.id, n.parent, n.name, n.kind)).collect()
+}
+
+/// A refused `component.createSet`: `Disabled`, no undo step, the document and the selection as they were.
+fn assert_create_set_refused(s: &mut Session, ids: &[NodeId]) {
+    let (before, undo, selection) = (snapshot(s), undo_len(s), s.selection());
+    let e = s.execute("component.createSet", &json!({"ids": ids})).unwrap_err();
+    assert!(matches!(e, crate::EngineError::Disabled(..)), "{e:?}");
+    assert_eq!(undo_len(s), undo, "no undo step");
+    assert_eq!(snapshot(s), before, "the document is unchanged");
+    assert_eq!(s.selection(), selection, "the selection is unchanged");
+}
+
+/// Nothing to wrap is refused before anything is created.
+#[test]
+fn create_set_refuses_no_layers_and_leaves_the_document_alone() {
+    let mut s = Session::new();
+    named_frames(&mut s, &["A"], false);
+    s.execute("select.set", &json!({"ids": []})).unwrap();
+    assert_create_set_refused(&mut s, &[]);
+}
+
+/// One layer is allowed: a set of one variant, as many layers would make.
+#[test]
+fn create_set_with_one_layer_makes_a_one_variant_set() {
+    let mut s = Session::new();
+    let frames = named_frames(&mut s, &["Solo"], false);
+    let before = undo_len(&s);
+    let out = s.execute("component.createSet", &json!({"ids": frames})).unwrap();
+    assert_eq!(undo_len(&s), before + 1, "one undo step");
+    let set = id_of(&out["id"]).unwrap();
+    let doc = s.doc().unwrap();
+    assert_eq!(doc.node(set).unwrap().children.len(), 1);
+    assert_eq!(names_by_x(&s, set), ["Property 1=Default"], "a lone layer shares its name with itself");
+    assert!(matches!(doc.node(doc.node(set).unwrap().children[0]).unwrap().kind, NodeKind::Component { .. }));
+    s.undo().unwrap();
+    assert!(matches!(s.doc().unwrap().node(frames[0]).unwrap().kind, NodeKind::Frame));
+}
+
+/// Each refusal branch of `create_set`, with `can_create_set` agreeing with it.
+#[test]
+fn create_set_refuses_every_ineligible_selection_and_leaves_the_document_alone() {
+    use crate::cmd::component::can_create_set;
+    let mut s = Session::new();
+    let frames = named_frames(&mut s, &["A", "B", "C"], false);
+    let comp = id_of(&s.execute("component.create", &json!({"ids": [frames[0]]})).unwrap()["id"]).unwrap();
+    let inst = id_of(&s.execute("instance.create", &json!({"component": comp})).unwrap()["id"]).unwrap();
+    let set_frames = named_frames(&mut s, &["V1", "V2"], false);
+    let set = id_of(&s.execute("component.createSet", &json!({"ids": set_frames})).unwrap()["id"]).unwrap();
+    // A layer inside the instance (its copy of the card's content).
+    let in_inst = s.doc().unwrap().node(inst).unwrap().children.first().copied();
+    // A library copy.
+    let remote = {
+        let doc = s.doc_mut().unwrap();
+        let store = doc.ensure_library_store();
+        let remote = doc.alloc_id();
+        doc.insert(Node::new(remote, NodeKind::Frame, "Remote", 10.0, 10.0), store, None).unwrap();
+        remote
+    };
+    let ok = frames[1];
+    assert!(can_create_set(s.doc().unwrap(), &[ok, frames[2]]));
+    assert!(!can_create_set(s.doc().unwrap(), &[]));
+    let mut cases = vec![("instance", inst), ("library copy", remote), ("component", comp), ("component set", set)];
+    if let Some(i) = in_inst {
+        cases.push(("inside an instance", i));
+    }
+    for (what, bad) in cases {
+        assert!(!can_create_set(s.doc().unwrap(), &[ok, bad]), "{what}: not offered");
+        assert_create_set_refused(&mut s, &[ok, bad]);
+        assert_create_set_refused(&mut s, &[bad]);
+    }
+}
+
+/// A command that fails leaves no trace: the checks before the creation loop catch every
+/// refusal the loop's own commands could hit, and `Session::execute` restores the document, the
+/// selection and the undo stack on any error, so no half-made components can survive.
+#[test]
+fn a_failing_create_set_leaves_no_components_behind() {
+    let mut s = Session::new();
+    let frames = named_frames(&mut s, &["A", "B"], false);
+    let (before, undo, selection) = (snapshot(&s), undo_len(&s), s.selection());
+    let e = s.execute("component.createSet", &json!({"ids": [frames[0], NodeId(u64::MAX)]}));
+    assert!(e.is_err());
+    assert_eq!(undo_len(&s), undo);
+    assert_eq!(snapshot(&s), before);
+    assert_eq!(s.selection(), selection);
+}

crates/ui-egui/src/create_multiple_menu_tests.rs

@@ -134,3 +134,32 @@ fn the_third_entry_creates_one_component_set() {
     let doc = u.app.session.doc().unwrap();
     assert!(ids.iter().all(|i| matches!(doc.node(*i).map(|n| &n.kind), Some(NodeKind::Frame))), "one undo reverts it all");
 }
+
+/// The component button's width in the Design panel: wider with a dropdown chevron beside the icon.
+fn button_width(u: &mut Ui) -> f32 {
+    let all = nodes(u);
+    let button = all.iter().find(|n| n.label().is_some_and(|l| l.starts_with("Create component"))).expect("the Design panel's component button");
+    bounds(button).width()
+}
+
+#[test]
+fn the_dropdown_chevron_is_there_only_for_several_layers() {
+    let (mut one, _) = frames(1);
+    let (mut two, _) = frames(2);
+    let (single, several) = (button_width(&mut one), button_width(&mut two));
+    assert_eq!(single, 24.0, "one layer: the plain icon button");
+    assert_eq!(several, 34.0, "several layers: room for the chevron");
+}
+
+#[test]
+fn create_component_set_is_offered_only_where_it_works() {
+    let (mut u, ids) = frames(3);
+    // Make the first a component: the set can't take it, so the entry is left out.
+    u.app.session.execute("component.create", &json!({"ids": [ids[0]]})).unwrap();
+    u.app.session.execute("select.set", &json!({"ids": ids})).unwrap();
+    u.frame(vec![]);
+    click_button(&mut u);
+    let all = nodes(&mut u);
+    assert!(all.iter().any(|n| n.label() == Some("Create multiple components")), "the menu is open");
+    assert!(all.iter().all(|n| n.label() != Some("Create component set")), "left out for a selection with a component");
+}

crates/ui-egui/src/panels/right.rs

@@ -494,7 +494,10 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
     // Layers in boolean groups, or one boolean group: the operation they use (checked in the menu).
     let current = figcraft_engine::booleans::shown_op(doc, sel);
     for (icon, label, cmd) in actions.into_iter().rev() {
-        let br = Rect::from_min_size(pos2(x - 24.0, r.center().y - 12.0), vec2(24.0, 24.0));
+        // With several layers the component button is a dropdown: a small chevron sits beside its icon.
+        let dropdown = cmd == "component.create" && sel.len() > 1;
+        let bw = if dropdown { 34.0 } else { 24.0 };
+        let br = Rect::from_min_size(pos2(x - bw, r.center().y - 12.0), vec2(bw, 24.0));
         let tip = if cmd == "bool.union" { label } else { crate::shortcuts::tip(label, cmd) };
         let bresp = ui.interact(br, ui.scope_id().with(("qa", cmd)), Sense::click()).tip(tip);
         if bresp.hovered() {
@@ -505,7 +508,11 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
             ("bool.union", Some(op)) => crate::layers::op_icon(op),
             _ => icon,
         };
-        icons::paint(ui.painter(), br.shrink(5.0), icon, t.icon);
+        let icon_rect = Rect::from_min_size(br.min, vec2(24.0, 24.0)).shrink(5.0);
+        icons::paint(ui.painter(), icon_rect, icon, t.icon);
+        if dropdown {
+            icons::paint(ui.painter(), Rect::from_center_size(pos2(br.max.x - 6.0, br.center().y), vec2(8.0, 8.0)), Icon::Chevron, t.icon_secondary);
+        }
         if cmd == "bool.union" {
             menu_popup(&bresp).width(180.0).show(|ui| {
                 for (op, c) in [
@@ -532,14 +539,24 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
             });
         } else if cmd == "component.create" && sel.len() > 1 {
             // Several layers: the button opens a menu, as Figma's does (#2088). One layer keeps the plain click.
-            menu_popup(&bresp).width(220.0).show(|ui| {
-                let multiple = crate::i18n::command("component.createMultiple", "Create multiple components");
-                let set = crate::i18n::command("component.createSet", "Create component set");
-                for (c, label) in
-                    [("component.create", tr("Create component")), ("component.createMultiple", &multiple), ("component.createSet", &set)]
-                {
+            let create = crate::i18n::command("component.create", "Create component");
+            let multiple = crate::i18n::command("component.createMultiple", "Create multiple components");
+            let set = crate::i18n::command("component.createSet", "Create component set");
+            // Like the layers menu, leave out what the selection can't use.
+            let can_set = figcraft_engine::cmd::component::can_create_set(doc, sel);
+            let mut entries = vec![("component.create", &create), ("component.createMultiple", &multiple)];
+            if can_set {
+                entries.push(("component.createSet", &set));
+            }
+            // Wide enough for the longest translated label (the rows grow with it, the menu never clips).
+            let widest = entries
+                .iter()
+                .map(|(_, l)| ui.painter().layout_no_wrap(l.to_string(), regular(11.5), Color32::PLACEHOLDER).size().x)
+                .fold(0.0f32, f32::max);
+            menu_popup(&bresp).width(menu::menu_row_width(widest, 0.0).ceil().max(160.0)).show(|ui| {
+                for (c, label) in &entries {
                     let key = crate::shortcuts::shortcut(c).unwrap_or("");
-                    if menu_icon_item(ui, None, None, label, key) {
+                    if menu_item(ui, label, key) {
                         app.run(c, json!({}));
                         ui.close();
                     }
@@ -548,7 +565,7 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) {
         } else if bresp.clicked() {
             app.run(cmd, json!({}));
         }
-        x -= 26.0;
+        x -= bw + 2.0;
     }
     crate::component_panel::instance_section(app, ui, doc, sel);
 }