Skip to content

Commit

Permalink
tree: Fix bug with rebasing over conflicted change (microsoft#22372)
Browse files Browse the repository at this point in the history
## Description

This fixes a bug where ModularChangeFamily's rebase method was ignoring
that the base changeset is conflicted.
  • Loading branch information
alex-pardes authored Sep 4, 2024
1 parent c487006 commit 72b0148
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ export class ModularChangeFamily
0x89a /* Unexpected destroys in change to invert */,
);

if ((change.change.constraintViolationCount ?? 0) > 0) {
if (hasConflicts(change.change)) {
return makeModularChangeset(
undefined,
undefined,
Expand Down Expand Up @@ -827,6 +827,10 @@ export class ModularChangeFamily
over: TaggedChange<ModularChangeset>,
revisionMetadata: RevisionMetadataSource,
): ModularChangeset {
if (hasConflicts(over.change)) {
return taggedChange.change;
}

const change = taggedChange.change;
const maxId = Math.max(change.maxId ?? -1, over.change.maxId ?? -1);
const idState: IdAllocationState = { maxId };
Expand Down Expand Up @@ -1845,7 +1849,7 @@ export function intoDelta(
const idAllocator = MemoizedIdRangeAllocator.fromNextId();
const rootDelta: Mutable<DeltaRoot> = {};

if ((change.constraintViolationCount ?? 0) === 0) {
if (!hasConflicts(change)) {
// If there are no constraint violations, then tree changes apply.
const fieldDeltas = intoDeltaImpl(
change.fieldChanges,
Expand Down
34 changes: 34 additions & 0 deletions packages/dds/tree/src/test/shared-tree/editing.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3124,6 +3124,40 @@ describe("Editing", () => {
expectJsonTree([tree, tree2], [{}]);
});
});

it("Rebase over conflicted change", () => {
const tree1 = makeTreeFromJsonSequence(["A", "B"]);
const tree2 = tree1.fork();

// Remove A
remove(tree1, 0, 1);

// This transaction will be conflicted after rebasing since the previous edit deletes the constrained node.
tree2.transaction.start();
tree2.editor.addNodeExistsConstraint({
parent: undefined,
parentField: rootFieldKey,
parentIndex: 0,
});

// Remove B
remove(tree2, 1, 1);
tree2.transaction.commit();

const tree3 = tree1.fork();

// This edit will be rebased over the conflicted transaction.
insert(tree3, 1, "C");

tree1.merge(tree2, false);
tree1.merge(tree3, false);

tree2.rebaseOnto(tree1);
tree3.rebaseOnto(tree1);

const expected = ["B", "C"];
expectJsonTree([tree1, tree2, tree3], expected);
});
});

it.skip("edit removed content", () => {
Expand Down

0 comments on commit 72b0148

Please sign in to comment.