Repository navigation
[fix] keep parent links after convertNode, do not share state with subtree copies - #38
Merged
mwasiluk merged 1 commit intoOct 31, 2026
Conversation
…btree 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base:
modernize(5fd01a4, after #35 and #37). Third of the stacked sd-model PRs: A (#35,merged) → B (#37, merged) → D (this, fixes) → C (lodash in the lockfile). One commit on top
of
modernize.Likely root cause of SilverDecisions/SilverDecisions#234 (see below).
Bugs
replaceNode(used byconvertNode) did not set$parentof the children to the new node;they kept pointing to the removed node.
cloneSubtree/cloneNodecopied nodes and edges shallowly: the copy shared$fieldStatus(and the nested computed values of the nodes) with the original.
Reproduced in the SilverDecisions app (
2f77351, demo page, through the TreeDesigner methods theUI calls) with sd-model 0.2.11 from npm:
p*10), T2], global codep = 1; convert C to adecision node, change the code to
p = 2, recompute: the payoff below the converted node stays10 (scope of D initialised from the removed node), expected 20.
pasted (the children were treated as separate subtrees).
sidebar does on input), recompute: the original edge is invalid too, its probability is not
evaluated and the original tree reports
invalidProbability,probabilityDoNotSumUpTo1.Related issue: SilverDecisions/SilverDecisions#234
The issue reports that nodes in a copied part of the tree keep an old value of a global variable
(
Day= 3200 after the global code was changed toDay=-3200) and do not see a variable addedlater (
Expert). In the attached file (saved by SilverDecisions 1.2.1) every node with the stalescope is a descendant of one chance node ("Sam") whose own scope is current — the pattern bug 1
leaves when that node's type was converted after its children existed: the children keep the
removed node as
$parent, and sd-computations initialises their scope from its frozen scope.2f77351: globalDay=3200, R → chance C → decision →terminal (payoff
Day); convert C, then change the code toDay=-3200/Expert=-20000andrecompute: the payoff stays 3200 and the scopes below C have
Day: 3200and noExpert— thesymptom of the issue. With this PR: -3200,
Expertpresent.the issue's steps mention only copying; so the conversion is the likely, not a proven, cause.
versions (the links are rebuilt on load), so files are not damaged. The issue can be closed
once SilverDecisions is released with this sd-model.
Fix
replaceNodesetse.childNode.$parent;cloneNodedeep-copiescomputedand$fieldStatus,cloneSubtreedeep-copies the edges'$fieldStatus. Values stay the same, only the sharing ends.Regression tests in
test/contract/data-model-edit.test.js(fail on 0.2.11 and on this branchwithout the fix, pass with it). CHANGELOG entry.
Results
npm run test:build(Node 24 and 22): contract tests standalone(.min) 58, module(.min) 59,source 57, consumer bundle 57, Jasmine 8/8; also with the unreleased sd-utils of
SilverDecisions/sd-utilsmodernize2a3af7c(--sd-utils=<tgz>).undo/redo and save/load of the edited diagram unchanged. Same with the sd-utils
2a3af7ctarball.
App.serializeof 13 diagram files identical to 0.2.11 (the fix does not changeloading/saving).
--no-save: sd-computations 196/196, sd-tree-designer 12/12,SilverDecisions build + Karma 37/37 (also with the sd-utils
2a3af7ctarball).Not changed (open question):
convertNodecopies only the name of the node, not itscode,foldedstate or computed values.Squash merge
[fix] keep parent links after convertNode, do not share state with subtree copies