Merge pull request #2354 from dotCooCoo/fix/table-story-target

Reject ambiguous table IDs across stories

crates/engine/src/cmd.rs

@@ -332,12 +332,16 @@ pub(crate) fn named_targets(spec: &CommandSpec, p: &Value) -> Option<Vec<ItemId>
 }
 
 /// The table a call names with `table`, for a command whose params document an optional `table`.
-/// `None` when the call names no table. [`table_story`] reads the `story` that goes with it.
-pub(crate) fn named_table(spec: &CommandSpec, p: &Value) -> Option<u64> {
+/// `None` when the call names no table. A `table` that is present but not a non-negative integer,
+/// `null` included, is an error. [`table_story`] reads the `story` that goes with it.
+pub(crate) fn named_table(spec: &CommandSpec, p: &Value, cmd: &str) -> Result<Option<u64>> {
     if !spec.params.split(|c: char| !c.is_ascii_alphanumeric() && c != '?').any(|w| w == "table?") {
-        return None;
+        return Ok(None);
+    }
+    match p.get("table") {
+        None => Ok(None),
+        Some(v) => v.as_u64().map(Some).ok_or_else(|| bad(cmd, "`table` must be a nonnegative integer")),
     }
-    p.get("table").and_then(Value::as_u64)
 }
 
 /// The `story` a table command's call names alongside `table`, or `None` when the call has no

crates/engine/src/cmd/table/registry.rs

@@ -407,6 +407,6 @@ fn table_specs() -> Vec<CommandSpec> {
             in_cell,
             |s, _| toggle_selection(s)
         ),
-        cmd!(query "table.get", "Get Table", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} → rows, columns, cells (text, spans, fill), options", in_table_or_ids, get),
+        cmd!(query "table.get", "Get Table", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} → rows, columns, cells (text, spans, fill), options. Supplied `story` and `table` values must be nonnegative integer IDs. When a table ID occurs in multiple stories, supply `story` with `table`.", in_table_or_ids, get),
     ]
 }

crates/engine/src/cmd/table/target.rs

@@ -41,8 +41,20 @@ pub(super) struct Tgt {
     pub(super) range: CellRange,
 }
 
-fn find_table(doc: &Document, id: u64) -> Option<StoryId> {
-    doc.stories.iter().find(|(_, s)| s.tables.contains_key(&id)).map(|(k, _)| *k)
+fn find_table(doc: &Document, id: u64, cmd: &str) -> Result<StoryId> {
+    let mut stories = doc.stories.iter().filter(|(_, story)| story.tables.contains_key(&id)).map(|(sid, _)| *sid);
+    let first = stories.next().ok_or_else(|| bad(cmd, format!("no table {id}")))?;
+    if stories.next().is_some() {
+        return Err(bad(cmd, format!("table {id} exists in multiple stories; specify `story`")));
+    }
+    Ok(first)
+}
+
+fn id_param(p: &Value, key: &str, cmd: &str) -> Result<Option<u64>> {
+    match p.get(key) {
+        None => Ok(None),
+        Some(value) => value.as_u64().map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be a nonnegative integer"))),
+    }
 }
 
 fn pair(p: &Value, many: &str, one: &str) -> Option<(usize, usize)> {
@@ -57,10 +69,13 @@ fn pair(p: &Value, many: &str, one: &str) -> Option<(usize, usize)> {
 pub(super) fn target(s: &Session, p: &Value, cmd: &str) -> Result<Tgt> {
     let st = s.doc()?;
     let doc = &st.doc;
-    let explicit_table = p.get("table").and_then(Value::as_u64);
+    let explicit_table = id_param(p, "table", cmd)?;
+    let explicit_story = table_story(p, cmd)?.map(StoryId);
     let (story, table, range) = if let Some(tid) = explicit_table {
-        let explicit_story = table_story(p, cmd)?.map(StoryId);
-        let sid = explicit_story.or_else(|| find_table(doc, tid)).ok_or_else(|| bad(cmd, format!("no table {tid}")))?;
+        let sid = match explicit_story {
+            Some(sid) => sid,
+            None => find_table(doc, tid, cmd)?,
+        };
         (sid, tid, None)
     } else if let Some(c) = st.selection.cells {
         (c.story, c.table, Some(c.range))

crates/engine/src/lib.rs

@@ -686,8 +686,8 @@ impl Session {
             // A table command that takes `story` and `table` acts on the table the call names, so
             // the selection doesn't matter: with nothing selected, the caret outside a table, or
             // selected rows the command can't act on, `{"table": 3}` still runs.
-            if let Some(table) = cmd::named_table(spec, params)
-                && cmd::has_doc(self).is_ok()
+            if cmd::has_doc(self).is_ok()
+                && let Some(table) = cmd::named_table(spec, params, id)?
             {
                 let story = cmd::table_story(params, id)?;
                 let st = self.doc()?;

crates/engine/src/tests/table.rs

@@ -17,6 +17,78 @@ fn check(s: &Session) {
     s.doc().unwrap().doc.check().unwrap();
 }
 
+#[test]
+fn table_id_without_story_rejects_multiple_matching_stories() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let first = s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "text"})).unwrap();
+    let first_story = first["story"].as_u64().unwrap();
+    let shared_id = s.execute("table.insert", &json!({"rows": 1, "cols": 1})).unwrap()["table"].as_u64().unwrap();
+    let second = s.execute("frame.create", &json!({"rect": [320, 36, 580, 200], "content": "text"})).unwrap();
+    let second_story = second["story"].as_u64().unwrap();
+    let second_id = s.execute("table.insert", &json!({"rows": 2, "cols": 1})).unwrap()["table"].as_u64().unwrap();
+    s.edit(|d, _| {
+        let story = d.story_mut(doc::StoryId(second_story)).unwrap();
+        let mut table = (*story.tables.remove(&second_id).unwrap()).clone();
+        table.id = shared_id;
+        story.tables.insert(shared_id, std::sync::Arc::new(table));
+        for para in &mut story.paras {
+            if para.table == Some(second_id) {
+                para.table = Some(shared_id);
+            }
+        }
+        Ok(())
+    })
+    .unwrap();
+    check(&s);
+
+    let error = s.execute("table.get", &json!({"table": shared_id})).unwrap_err();
+    assert!(error.to_string().contains("multiple stories"), "{error}");
+    assert!(error.to_string().contains("story"), "{error}");
+    assert_eq!(s.execute("table.get", &json!({"story": first_story, "table": shared_id})).unwrap()["rows"].as_array().unwrap().len(), 1);
+    assert_eq!(s.execute("table.get", &json!({"story": second_story, "table": shared_id})).unwrap()["rows"].as_array().unwrap().len(), 2);
+}
+
+#[test]
+fn table_target_rejects_wrong_typed_story_and_table_ids() {
+    let (mut s, story, _) = session_with_frame();
+    let table = s.execute("table.insert", &json!({"rows": 1, "cols": 1})).unwrap()["table"].as_u64().unwrap();
+    for (key, value) in [
+        ("story", json!("other")),
+        ("story", json!("")),
+        ("story", json!(-1)),
+        ("story", json!(1.5)),
+        ("story", Value::Null),
+        ("table", json!("other")),
+        ("table", json!("")),
+        ("table", json!(-1)),
+        ("table", json!(1.5)),
+        ("table", Value::Null),
+    ] {
+        let mut params = json!({"story": story, "table": table});
+        params[key] = value;
+        let error = s.execute("table.get", &params).unwrap_err();
+        assert!(error.to_string().contains(&format!("`{key}`")), "{key}: {error}");
+    }
+    let undo = s.doc().unwrap().history.undo.len();
+    let error = s.execute("table.setCell", &json!({"story": null, "table": table, "insets": 7})).unwrap_err();
+    assert!(error.to_string().contains("`story`"), "{error}");
+    assert_eq!(s.doc().unwrap().history.undo.len(), undo);
+    for params in [json!({"story": story, "table": u64::MAX}), json!({"story": u64::MAX, "table": table})] {
+        let error = s.execute("table.get", &params).unwrap_err();
+        assert!(error.to_string().contains("no table"), "{error}");
+    }
+    // With nothing in a table selected, a malformed `table` is refused as such, not as disabled.
+    s.execute("edit.deselectAll", &json!({})).unwrap();
+    for id in ["table.selectTable", "table.insertRow"] {
+        for value in [json!("other"), json!(-1), Value::Null] {
+            let error = s.execute(id, &json!({ "table": value })).unwrap_err();
+            assert!(!matches!(error, crate::EngineError::Disabled(..)) && error.to_string().contains("`table`"), "{id} {value}: {error}");
+        }
+    }
+    assert_eq!(s.doc().unwrap().history.undo.len(), undo);
+}
+
 #[test]
 fn table_text_replacement_floors_utf8_selection_and_allows_quote_typing() {
     for (original, (a, b), replacement, (after_a, after_b), quoted) in [

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

@@ -26,6 +26,7 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) {
     let cols = info["columns"].as_array().cloned().unwrap_or_default();
     let range = info["range"].clone();
     let (r0, c0) = (range["r0"].as_u64().unwrap_or(0) as usize, range["c0"].as_u64().unwrap_or(0) as usize);
+    let sid = info["story"].clone();
     let tid = info["table"].clone();
     let cell = info["cells"].as_array().and_then(|cs| cs.iter().find(|c| c["row"] == r0 && c["col"] == c0)).cloned().unwrap_or(Value::Null);
     let run = |app: &mut DesignApp, id: &str, p: Value| {
@@ -72,7 +73,7 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) {
                 let combo = craft_ui::ComboBox::from_id_salt(("tp_style", kind)).selected_text(&cur).width(130.0).show_ui(ui, |ui| {
                     for n in &names {
                         if ui.selectable_label(*n == cur, n).clicked() {
-                            run(app, &format!("style.{kind}.apply"), json!({"name": n, "table": tid}));
+                            run(app, &format!("style.{kind}.apply"), json!({"name": n, "story": sid, "table": tid, "rows": [range["r0"], range["r1"]], "cols": [range["c0"], range["c1"]]}));
                         }
                     }
                 });
@@ -103,10 +104,10 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) {
                 run(
                     app,
                     "table.insertRow",
-                    json!({"table": tid, "rows": [rows.len() - 1, rows.len() - 1], "where": "below", "count": n - rows.len()}),
+                    json!({"story": sid, "table": tid, "rows": [rows.len() - 1, rows.len() - 1], "where": "below", "count": n - rows.len()}),
                 );
             } else if n < rows.len() {
-                run(app, "table.deleteRow", json!({"table": tid, "rows": [n, rows.len() - 1]}));
+                run(app, "table.deleteRow", json!({"story": sid, "table": tid, "rows": [n, rows.len() - 1]}));
             }
         }
         caption(ui, crate::i18n::tr_context(&app.ui.language, "Columns", "table"));
@@ -116,10 +117,10 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) {
                 run(
                     app,
                     "table.insertColumn",
-                    json!({"table": tid, "cols": [cols.len() - 1, cols.len() - 1], "where": "right", "count": n - cols.len()}),
+                    json!({"story": sid, "table": tid, "cols": [cols.len() - 1, cols.len() - 1], "where": "right", "count": n - cols.len()}),
                 );
             } else if n < cols.len() {
-                run(app, "table.deleteColumn", json!({"table": tid, "cols": [n, cols.len() - 1]}));
+                run(app, "table.deleteColumn", json!({"story": sid, "table": tid, "cols": [n, cols.len() - 1]}));
             }
         }
         ui.end_row();

docs/mcp.md

@@ -185,6 +185,12 @@ In connected mode every tool maps to a control-channel method (`engine.execute`,
 what the agent does shows up live in the window. A failed request is not resent: the operation may have completed,
 so inspect the document before retrying. The next request re-establishes the connection.
 
+## Table targets
+
+Table IDs can repeat across stories. For `execute` commands that target an existing table, supply `story` with `table`
+when an ID repeats. An ambiguous table-only request returns an error. Supplied `story` and `table` values must be
+nonnegative integers; `null` and values of other types return an error.
+
 ## Coordinates
 
 Points (1/72 in), y down, in **spread** space: on a single-page spread the page's top-left is (0, 0); on a facing