Skip to content

tree: Fix bug with node existence revert constraint #27492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alex-pardes wants to merge 2 commits into
base: main
Choose a base branch
from
+42 −52
Open

tree: Fix bug with node existence revert constraint #27492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a18f67
Add failing test
alex-pardes Jun 5, 2026
461c8a4
Fix revert node existence constraint
alex-pardes Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 14 additions & 47 deletions packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,14 @@ export class ModularChangeFamily

const crossFieldKeys = this.makeCrossFieldKeyTable(invertedFields, invertedNodes);

const constraintState = newConstraintState(0);
this.updateConstraintsForFields(
invertedFields,
NodeAttachState.Attached,
constraintState,
invertedNodes,
);

return makeModularChangeset({
fieldChanges: invertedFields,
nodeChanges: invertedNodes,
Expand All @@ -790,8 +798,7 @@ export class ModularChangeFamily
crossFieldKeys,
maxId: genId.getMaxId(),
revisions: revInfos,
constraintViolationCount: change.change.constraintViolationCountOnRevert,
constraintViolationCountOnRevert: change.change.constraintViolationCount,
constraintViolationCount: constraintState.violationCount,
noChangeConstraint,
noChangeConstraintOnRevert,
destroys,
Expand Down Expand Up @@ -939,9 +946,6 @@ export class ModularChangeFamily
);

const constraintState = newConstraintState(change.constraintViolationCount ?? 0);
const revertConstraintState = newConstraintState(
change.constraintViolationCountOnRevert ?? 0,
);

let noChangeConstraint = change.noChangeConstraint;
if (
Expand All @@ -956,9 +960,7 @@ export class ModularChangeFamily
this.updateConstraintsForFields(
rebasedFields,
NodeAttachState.Attached,
NodeAttachState.Attached,
constraintState,
revertConstraintState,
rebasedNodes,
);

Expand All @@ -971,7 +973,6 @@ export class ModularChangeFamily
maxId: idState.maxId,
revisions: change.revisions,
constraintViolationCount: constraintState.violationCount,
constraintViolationCountOnRevert: revertConstraintState.violationCount,
noChangeConstraint,
noChangeConstraintOnRevert: change.noChangeConstraintOnRevert,
builds: change.builds,
Expand Down Expand Up @@ -1410,43 +1411,30 @@ export class ModularChangeFamily
private updateConstraintsForFields(
fields: FieldChangeMap,
parentInputAttachState: NodeAttachState,
parentOutputAttachState: NodeAttachState,
constraintState: ConstraintState,
revertConstraintState: ConstraintState,
nodes: ChangeAtomIdBTree<NodeChangeset>,
): void {
for (const field of fields.values()) {
const handler = getChangeHandler(this.fieldKinds, field.fieldKind);
for (const [nodeId, inputIndex, outputIndex] of handler.getNestedChanges(field.change)) {
for (const [nodeId, inputIndex, _outputIndex] of handler.getNestedChanges(
field.change,
Copy link
Copy Markdown
Contributor

@yann-achard-MS yann-achard-MS Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was the only code path where the output index was used. You could remove it from the getNestedChanges contract (and all computations of it) as part of this PR, or we could do that in a separate PR, I don't mind.

)) {
const isInputDetached = inputIndex === undefined;
const inputAttachState =
parentInputAttachState === NodeAttachState.Detached || isInputDetached
? NodeAttachState.Detached
: NodeAttachState.Attached;
const isOutputDetached = outputIndex === undefined;
const outputAttachState =
parentOutputAttachState === NodeAttachState.Detached || isOutputDetached
? NodeAttachState.Detached
: NodeAttachState.Attached;
this.updateConstraintsForNode(
nodeId,
inputAttachState,
outputAttachState,
nodes,
constraintState,
revertConstraintState,
);

this.updateConstraintsForNode(nodeId, inputAttachState, nodes, constraintState);
}
}
}

private updateConstraintsForNode(
nodeId: NodeId,
inputAttachState: NodeAttachState,
outputAttachState: NodeAttachState,
nodes: ChangeAtomIdBTree<NodeChangeset>,
constraintState: ConstraintState,
revertConstraintState: ConstraintState,
): void {
const node =
nodes.get([nodeId.revision, nodeId.localId]) ?? fail(0xb24 /* Unknown node ID */);
Expand All @@ -1460,24 +1448,12 @@ export class ModularChangeFamily
constraintState.violationCount += isNowViolated ? 1 : -1;
}
}
if (node.nodeExistsConstraintOnRevert !== undefined) {
const isNowViolated = outputAttachState === NodeAttachState.Detached;
if (node.nodeExistsConstraintOnRevert.violated !== isNowViolated) {
node.nodeExistsConstraintOnRevert = {
...node.nodeExistsConstraintOnRevert,
violated: isNowViolated,
};
revertConstraintState.violationCount += isNowViolated ? 1 : -1;
}
}

if (node.fieldChanges !== undefined) {
this.updateConstraintsForFields(
node.fieldChanges,
inputAttachState,
outputAttachState,
constraintState,
revertConstraintState,
nodes,
);
}
Expand Down Expand Up @@ -2003,7 +1979,6 @@ export function updateRefreshers(
maxId,
revisions,
constraintViolationCount,
constraintViolationCountOnRevert,
builds,
destroys,
} = change;
Expand All @@ -2017,7 +1992,6 @@ export function updateRefreshers(
maxId: maxId as number,
revisions,
constraintViolationCount,
constraintViolationCountOnRevert,
builds,
destroys,
refreshers,
Expand Down Expand Up @@ -2632,7 +2606,6 @@ function makeModularChangeset(props?: {
maxId: number;
revisions?: readonly RevisionInfo[];
constraintViolationCount?: number;
constraintViolationCountOnRevert?: number;
noChangeConstraint?: NoChangeConstraint;
noChangeConstraintOnRevert?: NoChangeConstraint;
builds?: ChangeAtomIdBTree<TreeChunk>;
Expand All @@ -2657,12 +2630,6 @@ function makeModularChangeset(props?: {
if (p.constraintViolationCount !== undefined && p.constraintViolationCount > 0) {
changeset.constraintViolationCount = p.constraintViolationCount;
}
if (
p.constraintViolationCountOnRevert !== undefined &&
p.constraintViolationCountOnRevert > 0
) {
changeset.constraintViolationCountOnRevert = p.constraintViolationCountOnRevert;
}
if (p.noChangeConstraint !== undefined) {
changeset.noChangeConstraint = p.noChangeConstraint;
}
Expand Down
5 changes: 0 additions & 5 deletions packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,6 @@ export interface ModularChangeset extends Readonly<HasFieldChanges> {
readonly noChangeConstraint?: NoChangeConstraint;
/** Constraint that the document must be in the same state before the revert of this change is applied as it was after this change was applied */
readonly noChangeConstraintOnRevert?: NoChangeConstraint;
/**
* The number of constraint violations that apply to the revert of the changeset. If this count is greater than 0, it will
* prevent the changeset from being reverted or undone.
*/
readonly constraintViolationCountOnRevert?: number;
readonly builds?: ChangeAtomIdBTree<TreeChunk>;
readonly destroys?: ChangeAtomIdBTree<number>;
readonly refreshers?: ChangeAtomIdBTree<TreeChunk>;
Expand Down
28 changes: 28 additions & 0 deletions packages/dds/tree/src/test/shared-tree/editing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3218,6 +3218,34 @@ describe("Editing", () => {

stack.unsubscribe();
});

it("inverse node existence constraint not violated by unrelated change on moved node", () => {
Copy link
Copy Markdown
Contributor

@yann-achard-MS yann-achard-MS Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it("inverse node existence constraint not violated by unrelated change on moved node", () => {
it("inverse node existence constraint on moved node not violated by unrelated change", () => {

const tree = makeTreeFromJsonSequence(["A", "B"]);
const branch = tree.fork();
const stack = createTestUndoRedoStacks(tree.events);

// Make transaction that does the following:
// 1. Moves "A" after "B".
// 2. Adds inverse constraint on existence of node "A".
branch.transaction.start();
branch.editor.move(rootField, 0, 1, rootField, 2);
branch.editor.addNodeExistsConstraintOnRevert(rootNode2);
branch.transaction.commit();
expectJsonTree(branch, ["B", "A"]);

insert(tree, 2, "C");
expectJsonTree(tree, ["A", "B", "C"]);

tree.merge(branch);
expectJsonTree(tree, ["B", "A", "C"]);
const moveRevertible = stack.undoStack[1] ?? assert.fail("Missing undo");

// The inverse constraint should not be violated.
moveRevertible.revert();
expectJsonTree(tree, ["A", "B", "C"]);

stack.unsubscribe();
});
});

it("Rebase over conflicted change", () => {
Expand Down
Loading