From 8441dc5c20a6b7920e47b6cf5a751f1086762a78 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 20:05:42 +0000 Subject: [PATCH] [fix] keep parent links after convertNode, do not share state with subtree copies replaceNode (used by convertNode) did not set $parent of the children to the new node, so they pointed to the replaced node that is no longer in the model. cloneSubtree copied nodes and edges shallowly and shared their $fieldStatus and the nested computed values of the nodes with the original. Regression tests in test/contract/data-model-edit.test.js. --- CHANGELOG.md | 15 ++++++++ src/data-model.js | 9 +++-- test/contract/data-model-edit.test.js | 51 +++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1dd8c4..ca84d7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ Changes since `0.2.11`. The version number is not changed yet, and the committed `dist/` is still the `0.2.11` build. +### Behaviour changes (bug fixes) + +- `DataModel.convertNode` (via `replaceNode`) sets `$parent` of the children + to the new node. They kept pointing to the replaced node, which is no + longer in the model: sd-computations then initialised the expression scope + of the children from that node (variable values of the time of the + conversion), and sd-tree-designer treated the children of a selected + converted node as separate subtrees (copy and paste duplicated them). + Saving and loading the diagram repaired the links. +- `DataModel.cloneSubtree` (copy / paste) copies the field status + (`$fieldStatus`) of the nodes and edges and the computed values of the + nodes deeply. The copy shared these objects with the original: marking a + field of the pasted copy as invalid also marked the original field, and + sd-computations then skipped the evaluation of the original's expression. + ### Build and tests - The bundles are built with Rollup and Babel 7 instead of gulp/browserify, diff --git a/src/data-model.js b/src/data-model.js index 800a097..3bdea9a 100644 --- a/src/data-model.js +++ b/src/data-model.js @@ -368,7 +368,10 @@ export class DataModel { } newNode.childEdges = oldNode.childEdges; - newNode.childEdges.forEach(e=>e.parentNode=newNode); + newNode.childEdges.forEach(e=>{ + e.parentNode=newNode; + e.childNode.$parent=newNode; + }); var index = this.nodes.indexOf(oldNode); if(~index){ @@ -394,6 +397,7 @@ export class DataModel { childClone.$parent = clone; var edge = Utils.clone(e); edge.id = Utils.guid(); + edge.$fieldStatus = Utils.cloneDeep(e.$fieldStatus); edge.parentNode = clone; edge.childNode = childClone; edge.payoff = Utils.cloneDeep(e.payoff); @@ -437,7 +441,8 @@ export class DataModel { var clone = Utils.clone(node) clone.id = Utils.guid(); clone.location = Utils.clone(node.location); - clone.computed = Utils.clone(node.computed); + clone.computed = Utils.cloneDeep(node.computed); + clone.$fieldStatus = Utils.cloneDeep(node.$fieldStatus); clone.$parent = null; clone.childEdges = []; return clone; diff --git a/test/contract/data-model-edit.test.js b/test/contract/data-model-edit.test.js index 60e216d..cbf5c3a 100644 --- a/test/contract/data-model-edit.test.js +++ b/test/contract/data-model-edit.test.js @@ -222,3 +222,54 @@ test('clear, clearComputedValues, clearExpressionScope, reversePayoffs, getGloba assert.deepEqual([dm.code, dm.$codeError, dm.$codeDirty, dm.defaultCriterion1Weight, dm.weightLowerBound, dm.weightUpperBound], ['', null, false, 1, 0, Infinity]); }); + +// Regression: replaceNode (used by convertNode) did not update $parent of the +// children, which kept pointing to the removed node. sd-computations then took +// the expression scope of the children from the removed node (stale variable +// values) and sd-tree-designer treated the children of a selected converted +// node as separate subtrees (copy / paste duplicated them). +test('convertNode: the children of the new node have it as $parent', () => { + const {dm, root, chance, toChance, toT1, toT2} = sampleTree(); + dm.convertNode(chance, 'decision'); + const converted = toChance.childNode; + assert.equal(toT1.childNode.$parent, converted); + assert.equal(toT2.childNode.$parent, converted); + checkIntegrity(assert, dm); + const subtree = dm.getAllNodesInSubtree(converted); + assert.deepEqual(dm.findSubtreeRoots(subtree), [converted]); + // a converted leaf keeps the parent chain + dm.convertNode(toT1.childNode, 'chance'); + assert.equal(toT1.childNode.$parent, converted); + checkIntegrity(assert, dm); + // and the relinked state survives undo / redo + dm.saveState(); + dm.removeNodes([converted]); + dm.undo(); + checkIntegrity(assert, dm); + assert.equal(dm.getRoots().length, 1); + assert.equal(dm.getRoots()[0].id, root.id); +}); + +// Regression: cloneSubtree shared the field status objects of the nodes and +// edges (and the nested computed values of the nodes) with the original, so +// marking a field of the pasted copy as invalid marked the original too, and +// sd-computations skipped evaluating the original's expressions. +test('cloneSubtree: the copy does not share field status or computed values with the original', () => { + const {dm, chance, toT1} = sampleTree(); + chance.computedValue('rule', 'payoff', 1); + chance.setValueValidity('name', true); + toT1.setValueValidity('probability', true); + for (const cloneComputed of [false, true]) { + const copy = dm.cloneSubtree(chance, cloneComputed); + const copiedEdge = copy.childEdges[0]; + copiedEdge.setSyntaxValidity('probability', false); + copy.setSyntaxValidity('name', false); + copy.computedValue('rule', 'payoff', 2); + assert.equal(toT1.isFieldValid('probability'), true); + assert.equal(chance.isFieldValid('name'), true); + assert.equal(chance.computedValue('rule', 'payoff'), 1); + // the copy keeps the state it was copied with + assert.equal(dm.cloneSubtree(copy).childEdges[0].isFieldValid('probability'), false); + assert.equal(dm.cloneSubtree(copy).computedValue('rule', 'payoff'), 2); + } +});