Implement centralized transactional fix for command batch failures (#229)
* Agent Host changes for agents/fix-command-batch-history-restore * Implement centralized transactional fix for command batch failures - #1 * The batch rollback tests compile, are formatted and expect the tab that was in front The new module used a private helper of plugins_tests and an unused import; the plugin test asserted tab 0 was in front, but it opens a second file first, which is the one in front when the plugin starts and the one the rollback puts back. --------- Co-authored-by: bflatastic <bombayapirat@gmail.com>
This commit is contained in:
Giuseppe Cautiero
GitHub
parent
f7e726714a
commit
ac65168417
|
|
@ -0,0 +1,63 @@
|
|||
use serde_json::json;
|
||||
|
||||
use crate::plugins_tests::{OnRefusal, install, manifest, script};
|
||||
use crate::{EngineError, Session};
|
||||
|
||||
fn id(value: &serde_json::Value) -> crate::NodeId {
|
||||
crate::params::id_of(&value["id"]).unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_group_closes_documents_opened_inside_it() {
|
||||
let mut s = Session::new();
|
||||
let original = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap());
|
||||
let before_journal = s.journal.clone();
|
||||
let before_history = s.ds().unwrap().history.undo.len();
|
||||
|
||||
let result = s.execute(
|
||||
"command.batch",
|
||||
&json!({
|
||||
"label": "Open then fail",
|
||||
"commands": [
|
||||
{"command": "file.new", "params": {"name": "Temporary"}},
|
||||
{"command": "shape.rectangle", "params": {"x": 20, "y": 20, "width": 10, "height": 10}},
|
||||
{"command": "node.set", "params": {"id": 999999, "props": {"name": "must fail"}}}
|
||||
]
|
||||
}),
|
||||
);
|
||||
|
||||
assert!(result.is_err());
|
||||
assert_eq!(s.docs.len(), 1);
|
||||
assert_eq!(s.active, 0);
|
||||
assert!(s.doc().unwrap().contains(original));
|
||||
assert_eq!(s.ds().unwrap().history.undo.len(), before_history);
|
||||
assert_eq!(s.journal, before_journal);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_plugin_run_restores_each_document_without_restoring_unrelated_state() {
|
||||
let mut s = Session::new();
|
||||
s.execute("file.new", &json!({"name": "Second"})).unwrap();
|
||||
let before_history: Vec<_> = s.docs.iter().map(|ds| ds.history.undo.len()).collect();
|
||||
let calls = [
|
||||
("shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})),
|
||||
("doc.nextTab", json!({})),
|
||||
("shape.rectangle", json!({"x": 20, "y": 0, "width": 10, "height": 10})),
|
||||
("node.set", json!({"id": 999999, "props": {"name": "must fail"}})),
|
||||
];
|
||||
install(&mut s, manifest("org.test.rollback", "write", &[]), &script(&calls, OnRefusal::Fail, ""));
|
||||
s.message = Some("keep this".into());
|
||||
let before_journal = s.journal.clone();
|
||||
let before_message = s.message.clone();
|
||||
// The new file is the one in front when the plugin starts.
|
||||
let before_active = s.active;
|
||||
|
||||
let result = s.execute("plugin.run", &json!({"id": "org.test.rollback"}));
|
||||
|
||||
assert!(matches!(result, Err(EngineError::Other(_))));
|
||||
assert_eq!(s.docs.iter().map(|ds| ds.doc.node(ds.page).map_or(0, |page| page.children.len())).collect::<Vec<_>>(), vec![0, 0]);
|
||||
assert_eq!(s.active, before_active, "the tab in front is put back");
|
||||
assert_eq!(s.docs.iter().map(|ds| ds.history.undo.len()).collect::<Vec<_>>(), before_history);
|
||||
assert_eq!(s.journal, before_journal);
|
||||
assert_eq!(s.message, before_message);
|
||||
}
|
||||
|
|
@ -1066,24 +1066,57 @@ impl Session {
|
|||
}
|
||||
|
||||
/// Run `f` as one undo step called `label`: the commands it executes push no steps of their
|
||||
/// own, and when it fails the document, selection, page and history are put back.
|
||||
/// own, and when it fails the documents stay at their group-start state.
|
||||
pub fn grouped<T>(&mut self, label: &str, f: impl FnOnce(&mut Self) -> Result<T>) -> Result<T> {
|
||||
let ds = self.ds_mut()?;
|
||||
let (before, sel, page, saved_len, uid) = (ds.doc.clone(), ds.selection.clone(), ds.page, ds.history.undo.len(), ds.uid);
|
||||
self.ds()?;
|
||||
let before: Vec<_> =
|
||||
self.docs.iter().map(|ds| (ds.uid, ds.doc.clone(), ds.selection.clone(), ds.page, ds.history.undo.len(), ds.history.limit)).collect();
|
||||
let active = self.active;
|
||||
let journal_len = self.journal.len();
|
||||
|
||||
// The steps inside are dropped again below, so they mustn't push the oldest ones out.
|
||||
let limit = std::mem::replace(&mut ds.history.limit, usize::MAX);
|
||||
self.live_hold(uid, true);
|
||||
for ds in &mut self.docs {
|
||||
ds.history.limit = usize::MAX;
|
||||
}
|
||||
for (uid, _, _, _, _, _) in &before {
|
||||
self.live_hold(*uid, true);
|
||||
}
|
||||
let result = f(self);
|
||||
self.live_hold(uid, false);
|
||||
let Some(ds) = self.docs.iter_mut().find(|d| d.uid == uid) else { return result };
|
||||
ds.history.limit = limit;
|
||||
ds.history.undo.truncate(saved_len);
|
||||
for (uid, _, _, _, _, _) in &before {
|
||||
self.live_hold(*uid, false);
|
||||
}
|
||||
|
||||
if result.is_err() {
|
||||
ds.doc = before;
|
||||
ds.selection = sel;
|
||||
ds.page = page;
|
||||
} else if !Arc::ptr_eq(&before, &ds.doc) {
|
||||
self.record(uid, Some(label), before, sel, page);
|
||||
self.docs.retain(|ds| before.iter().any(|(uid, _, _, _, _, _)| *uid == ds.uid));
|
||||
for (uid, doc, selection, page, undo_len, limit) in &before {
|
||||
if let Some(ds) = self.docs.iter_mut().find(|ds| ds.uid == *uid) {
|
||||
ds.doc = doc.clone();
|
||||
ds.selection = selection.clone();
|
||||
ds.page = *page;
|
||||
ds.history.undo.truncate(*undo_len);
|
||||
ds.history.limit = *limit;
|
||||
}
|
||||
}
|
||||
self.active = active;
|
||||
self.journal.truncate(journal_len);
|
||||
return result;
|
||||
}
|
||||
|
||||
let changed: Vec<_> = before
|
||||
.iter()
|
||||
.filter_map(|(uid, doc, selection, page, _, _)| {
|
||||
let ds = self.docs.iter().find(|d| d.uid == *uid)?;
|
||||
(!Arc::ptr_eq(doc, &ds.doc) && ds.interaction.is_none()).then(|| (*uid, doc.clone(), selection.clone(), *page))
|
||||
})
|
||||
.collect();
|
||||
for ds in &mut self.docs {
|
||||
if let Some((_, _, _, _, undo_len, limit)) = before.iter().find(|before| before.0 == ds.uid) {
|
||||
ds.history.limit = *limit;
|
||||
ds.history.undo.truncate(*undo_len);
|
||||
}
|
||||
}
|
||||
for (uid, doc, selection, page) in changed {
|
||||
self.record(uid, Some(label), doc, selection, page);
|
||||
}
|
||||
result
|
||||
}
|
||||
|
|
@ -1347,6 +1380,8 @@ mod arc_handles_tests;
|
|||
#[cfg(test)]
|
||||
mod autosave_tests;
|
||||
#[cfg(test)]
|
||||
mod batch_rollback_tests;
|
||||
#[cfg(test)]
|
||||
mod binding_lists_tests;
|
||||
#[cfg(test)]
|
||||
mod booleans_tests;
|
||||
|
|
@ -1477,7 +1512,7 @@ mod plugin_directory_tests;
|
|||
#[cfg(test)]
|
||||
mod plugin_panels_tests;
|
||||
#[cfg(test)]
|
||||
mod plugins_tests;
|
||||
pub(crate) mod plugins_tests;
|
||||
#[cfg(test)]
|
||||
mod progressive_blur_tests;
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ use figcraft_doc::NodeId;
|
|||
/// What a test plugin does when a call is refused: fail the run (status 1), or carry on and report
|
||||
/// the refusal as its output.
|
||||
#[derive(Clone, Copy, PartialEq)]
|
||||
enum OnRefusal {
|
||||
pub(crate) enum OnRefusal {
|
||||
Fail,
|
||||
Report,
|
||||
}
|
||||
|
|
@ -26,7 +26,7 @@ pub(crate) fn lit(s: &str) -> String {
|
|||
|
||||
/// A module that reads the selection, makes `calls` in order and outputs the selection it read
|
||||
/// (or the first refusal). `tail` runs after the calls (an endless loop, a trap…).
|
||||
fn script(calls: &[(&str, Value)], on_refusal: OnRefusal, tail: &str) -> Vec<u8> {
|
||||
pub(crate) fn script(calls: &[(&str, Value)], on_refusal: OnRefusal, tail: &str) -> Vec<u8> {
|
||||
let mut data = String::new();
|
||||
let mut body = String::from("(local.set $sel (call $reply_read (i32.const 60000) (call $selection)))\n");
|
||||
let mut at = 0usize;
|
||||
|
|
|
|||
|
|
@ -76,6 +76,36 @@ fn node_set_and_errors_roll_back() {
|
|||
assert!(s.execute("no.such", &json!({})).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_batch_restores_all_documents_and_history() {
|
||||
let mut s = sess();
|
||||
let original = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap());
|
||||
s.execute("file.new", &json!({"name": "Second"})).unwrap();
|
||||
s.execute("doc.nextTab", &json!({})).unwrap();
|
||||
let before_journal = s.journal.clone();
|
||||
let before_history: Vec<_> = s.docs.iter().map(|d| d.history.undo.len()).collect();
|
||||
|
||||
let result = s.execute(
|
||||
"command.batch",
|
||||
&json!({
|
||||
"label": "Broken batch",
|
||||
"commands": [
|
||||
{"command": "doc.nextTab"},
|
||||
{"command": "shape.rectangle", "params": {"x": 20, "y": 20, "width": 10, "height": 10}},
|
||||
{"command": "node.set", "params": {"id": 999999, "props": {"name": "must fail"}}}
|
||||
]
|
||||
}),
|
||||
);
|
||||
|
||||
assert!(result.is_err());
|
||||
assert_eq!(s.active, 0, "the active document is part of the transaction");
|
||||
assert_eq!(s.docs.len(), 2, "a failed batch cannot leave a newly opened tab behind");
|
||||
assert_eq!(s.doc().unwrap().node(original).unwrap().name, "Rectangle");
|
||||
assert_eq!(s.docs[1].doc.node(s.docs[1].page).map_or(0, |page| page.children.len()), 0);
|
||||
assert_eq!(s.docs.iter().map(|d| d.history.undo.len()).collect::<Vec<_>>(), before_history);
|
||||
assert_eq!(s.journal, before_journal, "failed inner commands are not journaled");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn group_ungroup_and_frame() {
|
||||
let mut s = sess();
|
||||
|
|
|
|||
|
|
@ -216,7 +216,7 @@ Or in a project's `.mcp.json`:
|
|||
|---|---|
|
||||
| `list_commands {filter?}` | Every command: id, label, menu, shortcut, undoable, params doc. |
|
||||
| `run_command {command, params}` | Run any command (e.g. `align.left`, `layout.add`, `component.create`, `instance.place` (drop an instance at a page point into the frame or auto layout slot under it), `component.exposeInstance` / `instance.exposed` (nested instance properties on instances), `component.makeSlot` / `instance.slotInsert` / `instance.resetSlot` (slots, below), `inspect.code` (CSS, HTML + CSS for a whole frame, React, SwiftUI or Jetpack Compose for a layer; components take their properties as parameters and instances are calls, see Dev Mode below), `inspect.properties` (layout, auto layout, typography, fills, strokes and effects with their style and variable names; `colorFormat`: hex, rgb, css, hsl or hsb), `inspect.assets` (layers under the selection with export settings, icons and images, each with its `kind`), `inspect.code` takes `unit: rem` and `rootFontSize` for CSS, HTML and React, `style.create`, `variable.bind`, `variable.update` (rename, describe, scope), `variable.set` with `{alias}`, `variable.renameMode` / `variable.removeMode`, `variable.deleteCollection`, `proto.interaction.add`, `codeConnect.set` (components linked to real code, see Dev Mode below)). |
|
||||
| `batch {commands: [{command, params}], label?}` | Several commands as one undo step (`command.batch`); all-or-nothing. |
|
||||
| `batch {commands: [{command, params}], label?}` | Several commands as one undo step (`command.batch`); all-or-nothing for open documents and their document state. Failed inner commands are removed from the journal; preferences, clipboard, tool state and UI requests are not rolled back. |
|
||||
| `create_node {type, x, y, width, height, parent, index, name, text, path, props}` | Create a frame, rectangle, ellipse, polygon, star, line, arrow, text, vector, section, component or slice; returns `{id, node}`. |
|
||||
| `set_properties {ids, props}` | `node.set` on layers (geometry, fills, strokes, effects, corner radius, auto layout, sizing, constraints, masks (`isMask`, `maskType: alpha\|vector\|luminance`), handoff status (`devStatus: none\|readyForDev\|completed`; marking again clears "Changed"), a section's `sectionContentsHidden`, text…); returns the updated layers. |
|
||||
| `inspect_document {depth, allPages}` | The document as JSON (layer tree, styles, selection, history). |
|
||||
|
|
|
|||
Loading…
Reference in New Issue