Fix incorrect indexes in TreeChange (#837)

This commit addresses the issue where indexes of TreeChange returned
during Tree.Style are inaccurately calculated. When executing
Tree.Style, users provide a range, but the actual execution is based
on nodes. This commit corrects the calculation of the TreeChange index
by basing it on nodes, resolving the issue.

---------

Co-authored-by: Youngteac Hong <susukang98@gmail.com>

src/api/converter.ts

@@ -1059,13 +1059,14 @@ function fromTreeNodes(
 function fromTreeNode(pbTreeNode: PbTreeNode): CRDTTreeNode {
   const id = fromTreeNodeID(pbTreeNode.id!);
   const node = CRDTTreeNode.create(id, pbTreeNode.type);
+  const pbAttrs = Object.entries(pbTreeNode.attributes);
   if (node.isText) {
     node.value = pbTreeNode.value;
-  } else {
+  } else if (pbAttrs.length) {
     const attrs = RHT.create();
-    Object.entries(pbTreeNode.attributes).forEach(([key, value]) => {
+    for (const [key, value] of pbAttrs) {
       attrs.set(key, value.value, fromTimeTicket(value.updatedAt)!);
-    });
+    }
     node.attrs = attrs;
   }
 
@@ -1203,6 +1204,7 @@ function fromOperation(pbOperation: PbOperation): Operation | undefined {
         fromTimeTicket(pbTreeStyleOperation!.parentCreatedAt)!,
         fromTreePos(pbTreeStyleOperation!.from!),
         fromTreePos(pbTreeStyleOperation!.to!),
+        createdAtMapByActor,
         attributesToRemove,
         fromTimeTicket(pbTreeStyleOperation!.executedAt)!,
       );

src/document/crdt/tree.ts

@@ -212,12 +212,12 @@ export class CRDTTreePos {
   }
 
   /**
-   * `toTreeNodes` converts the pos to parent and left sibling nodes.
+   * `toTreeNodePair` converts the pos to parent and left sibling nodes.
    * If the position points to the middle of a node, then the left sibling node
    * is the node that contains the position. Otherwise, the left sibling node is
    * the node that is located at the left of the position.
    */
-  public toTreeNodes(tree: CRDTTree): [CRDTTreeNode, CRDTTreeNode] {
+  public toTreeNodePair(tree: CRDTTree): TreeNodePair {
     const parentID = this.getParentID();
     const leftSiblingID = this.getLeftSiblingID();
     const parentNode = tree.findFloorNode(parentID);
@@ -409,6 +409,12 @@ export type CRDTTreeNodeIDStruct = {
  */
 export type TreePosRange = [CRDTTreePos, CRDTTreePos];
 
+/**
+ * `TreeNodePair` represents a pair of CRDTTreeNode. It represents the position
+ * of the node in the tree with the left and parent nodes.
+ */
+type TreeNodePair = [CRDTTreeNode, CRDTTreeNode];
+
 /**
  * `TreePosStructRange` represents the structure of TreeRange.
  * It is used to serialize and deserialize the TreeRange.
@@ -689,7 +695,7 @@ export class CRDTTreeNode
 }
 
 /**
- * toTreeNode converts the given CRDTTreeNode to TreeNode.
+ * `toTreeNode` converts the given CRDTTreeNode to TreeNode.
  */
 function toTreeNode(node: CRDTTreeNode): TreeNode {
   if (node.isText) {
@@ -700,17 +706,20 @@ function toTreeNode(node: CRDTTreeNode): TreeNode {
     } as TextNode;
   }
 
-  return {
+  const treeNode: TreeNode = {
     type: node.type,
     children: node.children.map(toTreeNode),
-    attributes: node.attrs
-      ? parseObjectValues(node.attrs?.toObject())
-      : undefined,
   };
+
+  if (node.attrs) {
+    treeNode.attributes = parseObjectValues(node.attrs?.toObject());
+  }
+
+  return treeNode;
 }
 
 /**
- * toXML converts the given CRDTNode to XML string.
+ * `toXML` converts the given CRDTNode to XML string.
  */
 export function toXML(node: CRDTTreeNode): string {
   if (node.isText) {
@@ -819,9 +828,9 @@ export class CRDTTree extends CRDTElement implements GCParent {
   public findNodesAndSplitText(
     pos: CRDTTreePos,
     editedAt?: TimeTicket,
-  ): [CRDTTreeNode, CRDTTreeNode] {
+  ): TreeNodePair {
     // 01. Find the parent and left sibling node of the given position.
-    const [parent, leftSibling] = pos.toTreeNodes(this);
+    const [parent, leftSibling] = pos.toTreeNodePair(this);
     let leftNode = leftSibling;
 
     // 02. Determine whether the position is left-most and the exact parent
@@ -912,13 +921,16 @@ export class CRDTTree extends CRDTElement implements GCParent {
             {},
           );
 
+          const parentOfNode = node.parent!;
+          const previousNode = node.prevSibling || node.parent!;
+
           if (Object.keys(affectedAttrs).length > 0) {
             changes.push({
               type: TreeChangeType.Style,
-              from: this.toIndex(fromParent, fromLeft),
-              to: this.toIndex(toParent, toLeft),
-              fromPath: this.toPath(fromParent, fromLeft),
-              toPath: this.toPath(toParent, toLeft),
+              from: this.toIndex(parentOfNode, previousNode),
+              to: this.toIndex(node, node),
+              fromPath: this.toPath(parentOfNode, previousNode),
+              toPath: this.toPath(node, node),
               actor: editedAt.getActorID(),
               value: affectedAttrs,
             });
@@ -943,22 +955,37 @@ export class CRDTTree extends CRDTElement implements GCParent {
     range: [CRDTTreePos, CRDTTreePos],
     attributesToRemove: Array<string>,
     editedAt: TimeTicket,
-  ): [Array<GCPair>, Array<TreeChange>] {
+    maxCreatedAtMapByActor?: Map<string, TimeTicket>,
+  ): [Map<string, TimeTicket>, Array<GCPair>, Array<TreeChange>] {
     const [fromParent, fromLeft] = this.findNodesAndSplitText(
       range[0],
       editedAt,
     );
     const [toParent, toLeft] = this.findNodesAndSplitText(range[1], editedAt);
 
     const changes: Array<TreeChange> = [];
+    const createdAtMapByActor = new Map<string, TimeTicket>();
     const pairs: Array<GCPair> = [];
     this.traverseInPosRange(
       fromParent,
       fromLeft,
       toParent,
       toLeft,
       ([node]) => {
-        if (!node.isRemoved && !node.isText && attributesToRemove) {
+        const actorID = node.getCreatedAt().getActorID();
+        const maxCreatedAt = maxCreatedAtMapByActor
+          ? maxCreatedAtMapByActor!.has(actorID)
+            ? maxCreatedAtMapByActor!.get(actorID)!
+            : InitialTimeTicket
+          : MaxTimeTicket;
+
+        if (node.canStyle(editedAt, maxCreatedAt) && attributesToRemove) {
+          const maxCreatedAt = createdAtMapByActor!.get(actorID);
+          const createdAt = node.getCreatedAt();
+          if (!maxCreatedAt || createdAt.after(maxCreatedAt)) {
+            createdAtMapByActor.set(actorID, createdAt);
+          }
+
           if (!node.attrs) {
             node.attrs = new RHT();
           }
@@ -970,20 +997,23 @@ export class CRDTTree extends CRDTElement implements GCParent {
             }
           }
 
+          const parentOfNode = node.parent!;
+          const previousNode = node.prevSibling || node.parent!;
+
           changes.push({
             actor: editedAt.getActorID()!,
             type: TreeChangeType.RemoveStyle,
-            from: this.toIndex(fromParent, fromLeft),
-            to: this.toIndex(toParent, toLeft),
-            fromPath: this.toPath(fromParent, fromLeft),
-            toPath: this.toPath(toParent, toLeft),
+            from: this.toIndex(parentOfNode, previousNode),
+            to: this.toIndex(node, node),
+            fromPath: this.toPath(parentOfNode, previousNode),
+            toPath: this.toPath(node, node),
             value: attributesToRemove,
           });
         }
       },
     );
 
-    return [pairs, changes];
+    return [createdAtMapByActor, pairs, changes];
   }
 
   /**

src/document/json/tree.ts

@@ -371,7 +371,7 @@ export class Tree {
     const toPos = this.tree.findPos(toIdx);
     const ticket = this.context.issueTimeTicket();
 
-    const [pairs] = this.tree!.removeStyle(
+    const [maxCreationMapByActor, pairs] = this.tree!.removeStyle(
       [fromPos, toPos],
       attributesToRemove,
       ticket,
@@ -386,6 +386,7 @@ export class Tree {
         this.tree.getCreatedAt(),
         fromPos,
         toPos,
+        maxCreationMapByActor,
         attributesToRemove,
         ticket,
       ),

src/document/operation/operation.ts

@@ -150,7 +150,7 @@ export type TreeEditOpInfo = {
   from: number;
   to: number;
   value?: Array<TreeNode>;
-  splitLevel: number;
+  splitLevel?: number;
   fromPath: Array<number>;
   toPath: Array<number>;
 };
@@ -164,7 +164,14 @@ export type TreeStyleOpInfo = {
   from: number;
   to: number;
   fromPath: Array<number>;
-  value: { [key: string]: any };
+  toPath: Array<number>;
+  value:
+    | {
+        attributes: Indexable;
+      }
+    | {
+        attributesToRemove: Array<string>;
+      };
 };
 
 /**

src/document/operation/tree_style_operation.ts

@@ -86,14 +86,15 @@ export class TreeStyleOperation extends Operation {
     parentCreatedAt: TimeTicket,
     fromPos: CRDTTreePos,
     toPos: CRDTTreePos,
+    maxCreatedAtMapByActor: Map<string, TimeTicket>,
     attributesToRemove: Array<string>,
     executedAt: TimeTicket,
   ): TreeStyleOperation {
     return new TreeStyleOperation(
       parentCreatedAt,
       fromPos,
       toPos,
-      new Map(),
+      maxCreatedAtMapByActor,
       new Map(),
       attributesToRemove,
       executedAt,
@@ -127,10 +128,11 @@ export class TreeStyleOperation extends Operation {
     } else {
       const attributesToRemove = this.attributesToRemove;
 
-      [pairs, changes] = tree.removeStyle(
+      [, pairs, changes] = tree.removeStyle(
         [this.fromPos, this.toPos],
         attributesToRemove,
         this.getExecutedAt(),
+        this.maxCreatedAtMapByActor,
       );
     }
 
@@ -139,13 +141,16 @@ export class TreeStyleOperation extends Operation {
     }
 
     return {
-      opInfos: changes.map(({ from, to, value, fromPath }) => {
+      opInfos: changes.map(({ from, to, value, fromPath, toPath }) => {
         return {
           type: 'tree-style',
           from,
           to,
-          value,
+          value: this.attributes.size
+            ? { attributes: value }
+            : { attributesToRemove: value },
           fromPath,
+          toPath,
           path: root.createPath(this.getParentCreatedAt()),
         } as OperationInfo;
       }),

src/util/index_tree.ts

@@ -203,6 +203,19 @@ export abstract class IndexTreeNode<T extends IndexTreeNode<T>> {
     return undefined;
   }
 
+  /**
+   * `prevSibling` returns the previous sibling of the node.
+   */
+  get prevSibling(): T | undefined {
+    const offset = this.parent!.findOffset(this as any);
+    const sibling = this.parent!.children[offset - 1];
+    if (sibling) {
+      return sibling;
+    }
+
+    return undefined;
+  }
+
   /**
    * `isRemoved` returns true if the node is removed.
    */

test/integration/tree_concurrency_test.ts

@@ -604,7 +604,7 @@ describe('Tree.concurrency', () => {
     const content: TreeNode = {
       type: 'p',
       children: [{ type: 'text', value: 'd' }],
-      attributes: { italic: 'true' },
+      attributes: { italic: 'true', color: 'blue' },
     };
 
     const rangesArr = [
@@ -675,7 +675,7 @@ describe('Tree.concurrency', () => {
         StyleOpCode.StyleRemove,
         'color',
         '',
-        'remove-bold',
+        'remove-color',
       ),
       new StyleOperationType(
         RangeSelector.RangeAll,