Skip to content

Conversation

@KMSstudio
Copy link
Member

@KMSstudio KMSstudio commented Sep 15, 2025

What this PR does / why we need it?

Scope note: This PR covers undo for text edit operations only. It does not handle style operations.
Adding undo support for EditOperation in CRDTText. Like other undoable operations, when an edit operation is executed, it returns an ExecutionResult that includes the corresponding undo operation.

Implementation Idea

Consider the string "abcd", and an operation that deletes "bc" and inserts "123". The visible text changes from "abcd" to "a123d". To revert to the original, at least three approaches are conceivable:

  • Way 1. Remove three letters after "a" and insert "bc"
  • Way 2. Remove the substring "123" and insert "bc"
  • Way 3. Remove the second through fourth characters and insert "bc"

This PR implements Way 3. While this may not always be optimal, implementing Way 1 or Way 2 leads to reference issues (Reference Stability Issues), so this PR chose Way 3.

Note

Starting from (abcd)[a@1:0], apply an operation that deletes "bc" and inserts "123" (call this opA). After opA, the internal composition becomes:

(abcd)[a@1:0]
(a)[a@1:0] - (123)[a@2:0] - (d)[a@2:3]

The node (bc)[a@1:1, X] may or may not be present depending on synchronization status.

Implementation Details

The PR adopt second through fourth characters semantics.

To support this, introducing a function:

normalizePos(RGATreeSplitPos): RGATreeSplitPos

This function converts a given RGATreeSplitPos into a relative-offset form by computing how many characters from the start” the position represents, and stores that count in relativeOffset.

- **Example**  
  Chain: `|"12" 1:0| - |"A" 2:1| - |"34" 1:2|`  
  `normalizePos((1:2, rel=1)) -> (0:0, 4)`  
  Because `|"12"|=2 + |"A"|=1 + rel(1) = 4`

The inverse transformation is provided by:

refinePos(relative-form RGATreeSplitPos): RGATreeSplitPos

There might be a better name on two functions. Using the stored "Nth character" information, it reconstructs the original RGATreeSplitPos.

Execution → Undo Flow

  1. Edit executes.
  2. We normalizePos the range of the newly inserted substring, determine that the range to remove for undo is (for example) offset [2, 5), and record this interval inside the UndoOp.
  3. Undo execites.
  4. At undo time, we refinePos the recorded offset [2, 5) back into concrete positions.
  5. Execute the UndoOp with the recovered range (type=EditOperation).

Note: The generated id would be 0:0.

Reference Stability Issues

Consider the following sequence:

   : text
op1: text1
op2: text12
op3: text123
undo3: text12
undo2: text1
undo1: text

It is reasonable to assume:

  • At the time undoN executes, the visible plain text equals the result right after opN executed.

However, we cannot assume:

  • At the time undoN executes, the internal state (node ids, split states, etc.) equals the state right after opN executed.

Between opN and undoN, a variety of edits and undos may occur that change node ids and split structures:

  • A user might delete a text node and create a new one → the node id changes.
  • A user might create and later delete a new node → a previously single node may be split into two.
  • A remote user may concurrently edit.

Therefore, if Way 1/2 stores id:offset references for precise nodes, those references can become invalid or point elsewhere. For example, in "a123d", the "123" node’s id might be a:2@0 at forward-op execution time, but become b:1@0 by the time we execute the inverse op.

A possible approach is to introduce a gcLock: if a node id like a:2@0 is referenced by an undo op, we forcibly prevent its garbage collection so the referenced node is guaranteed to exist.

However, adopting this approach would cause most of the existing CRDT text GC tests to fail, so extensive test rewrites would be required. To avoid this, we chose Way 3.

Stack Reconcile

Because offsets can shift over time, we periodically reconcile fromPos, toPos in undoOperation of Edit.

For example, the offset 7 pointing to the character "1" in abcdef123g should become 5 if "ab" is deleted (cdef123g). Such adjustments are performed via:

EditOperation.reconsileOperation()

This keeps the stored intervals consistent with the current text.

Performance Considerations

Like array undo, we linearly scan the undo stack. If the stack grows to tens of thousands of entries, performance degradation is possible. We assume typical undo stack sizes are smaller; hence the current linear approach.

If a hard stack size limit is required, or if performance becomes an issue, we may switch to an auxiliary map-based index to avoid linear scans—bearing in mind the overhead this would introduce.

Any background context you want to provide?

Introduce another reconcile function to UndoStk for this feature.

What are the relevant tickets?

Fixes #

Related with Yorkie Iss#652

Checklist

  • Added relevant tests or not required
  • Addressed and resolved all CodeRabbit review comments
  • Didn't break anything

Summary by CodeRabbit

  • New Features

    • Collaborative undo/redo: keyboard shortcuts (Cmd/Ctrl+Z, Cmd/Ctrl+Y/Shift+Cmd/Ctrl+Z) now operate on shared history across clients in the CodeMirror example.
    • Improved undo/redo integration so undos/redos from any client apply consistently to the editor view.
  • Bug Fixes

    • Prevented duplicate/recursive edits during sync, improving multi-user stability.
  • Tests

    • Added integration tests for single, chained, and multi-client text undo scenarios.
  • Chores

    • Added a runtime dependency to support the enhanced editor behavior.

@coderabbitai
Copy link

coderabbitai bot commented Sep 15, 2025

Walkthrough

Adds multi-user undo/redo: routes editor undo/redo through Yorkie history, augments CRDT edit returns with removed values, adds position utilities (normalize/refine), reconciles text edits into history stacks, updates document reconciliation, and adds integration tests and example edits.

Changes

Cohort / File(s) Summary
Example: Vanilla CodeMirror6
examples/vanilla-codemirror6/package.json, examples/vanilla-codemirror6/src/main.ts
Adds @codemirror/view dependency; introduces CodeMirror keymap interception for Mod-Z/Mod-Y/Mod-Shift-Z to call doc.history.undo()/redo(); adapts local/remote event filtering and editor setup to handle undoredo-sourced changes.
SDK HTML example
packages/sdk/index.html
Routes editor undo/redo through Yorkie when available; treats undoredo events as content updates; tightens local-change filtering to avoid recursive edits.
CRDT core: RGATreeSplit
packages/sdk/src/document/crdt/rga_tree_split.ts
edit now returns removed values array; adds normalizePos and refinePos utilities for position normalization/refinement; updates related internals to collect removed values.
CRDT Text wrapper
packages/sdk/src/document/crdt/text.ts
Propagates additional removed-values return from RGATreeSplit.edit; updates edit return shape; adds refinePos and normalizePos delegating to RGATreeSplit.
Document apply/reconcile
packages/sdk/src/document/document.ts
After applying local/remote changes, iterates ops to reconcile createdAt for ArraySet and text-edit ranges for EditOperation (using normalizePos) via history reconciliation.
History reconciliation
packages/sdk/src/document/history.ts
Adds reconcileTextEdit(parentCreatedAt, rangeFrom, rangeTo, contentLen) to adjust undo/redo stacks for overlapping text edits.
Edit operation enhancements
packages/sdk/src/document/operation/edit_operation.ts
Adds undo-awareness (isUndoOp), optional executedAt, position normalization (normalizePos), range reconciliation (reconcileOperation), reverse-op generation from removed values, and returns reverseOp in execution result.
Integration tests
packages/sdk/test/integration/history_text_test.ts
New tests validating undo semantics: single op, chained ops (snapshots), and multi-client undo convergence using doc.history.undo().

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Editor as CodeMirror
  participant Keymap
  participant Doc as Yorkie Document
  participant Sync as Server/Peers

  User->>Editor: press Mod-Z / Mod-Y
  Editor->>Keymap: key event
  alt Yorkie available (intercept)
    Keymap->>Doc: history.undo()/history.redo()
    Doc->>Doc: apply reverse op(s) (isUndoOp)
    Doc-->>Editor: emit change (source=undoredo)
    Editor->>Editor: apply transaction (remote/undoredo)
    Doc->>Sync: sync change
    Sync-->>Doc: remote updates
  else Fallback to editor
    Keymap->>Editor: default undo/redo
  end
Loading
sequenceDiagram
  participant Client
  participant Document
  participant Op as EditOperation
  participant CRDT as CRDTText/RGATreeSplit
  participant History

  Client->>Document: apply/update change
  Document->>Op: execute(...)
  Op->>CRDT: edit(range, content)
  CRDT-->>Op: [pos, gcPairs, size, valueChanges, removedValues]
  Op->>Op: build reverseOp from removedValues
  Op-->>Document: ExecutionResult{reverseOp}
  Document->>History: reconcileTextEdit(parentCreatedAt, rangeFrom, rangeTo, contentLen)
  Note over History: adjust undo/redo stacks for overlapping edits
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • hackerwins

Poem

I tapped Mod‑Z with twitching ear,
The timeline wiggled, then grew clear.
Removed bits stitched back in rows,
History hops where collaboration goes.
A rabbit’s nudge — undo that glows. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Text undo: add functionality that can undo edit operation, which comtain replace operation, insert operation and remove operation." is directly related to the main change in the changeset. The core objective of this PR is to add text undo functionality by introducing undo support infrastructure (isUndoOp flag, reverse operation generation, position normalization/reconciliation methods) and routing undo/redo operations through Yorkie's history instead of CodeMirror's default behavior. The title clearly communicates this primary objective. While the title is somewhat verbose and contains a minor typo ("comtain" should be "contain"), it still effectively conveys the main change to someone scanning the git history, making it sufficiently clear and specific despite the verbosity.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-text-undo-KMS

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 78.00000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.93%. Comparing base (b11b4a0) to head (dc26454).

Files with missing lines Patch % Lines
...kages/sdk/src/document/operation/edit_operation.ts 72.22% 8 Missing and 7 partials ⚠️
packages/sdk/src/document/crdt/rga_tree_split.ts 77.77% 3 Missing and 3 partials ⚠️
packages/sdk/src/document/document.ts 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
- Coverage   77.95%   77.93%   -0.02%     
==========================================
  Files          67       67              
  Lines        5888     5983      +95     
  Branches     1065     1090      +25     
==========================================
+ Hits         4590     4663      +73     
- Misses        975      986      +11     
- Partials      323      334      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KMSstudio KMSstudio marked this pull request as ready for review September 15, 2025 05:50
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (6)
packages/sdk/src/document/history.ts (1)

166-170: Consider adding null checks for defensive programming.

While the current implementation works, consider adding checks to ensure op has the expected methods before calling them.

           if (
             op instanceof EditOperation &&
+            op.getParentCreatedAt &&
             op.getParentCreatedAt().compare(parentCreatedAt) === 0
           ) {
+            if (op.reconcileOperation) {
               op.reconcileOperation(rangeFrom, rangeTo, contentLen);
+            }
           }
packages/sdk/test/integration/history_text_test.ts (4)

5-6: Add 'style' to the ops array or document the reason for its omission.

The TextOp type includes 'style', but the ops array only includes ['insert', 'delete', 'replace']. This inconsistency could lead to confusion or missed test coverage.

-const ops: Array<TextOp> = ['insert', 'delete', 'replace'];
+const ops: Array<TextOp> = ['insert', 'delete', 'replace', 'style'];

If 'style' is intentionally excluded from certain test scenarios, consider adding a comment explaining why.


11-52: Consider making the operation set deterministic for edge case testing.

The applyTextOp1 function has branching logic based on text length that could lead to inconsistent test behavior. For thorough edge case testing, consider making operations more deterministic.

For example, the 'replace' operation behavior changes based on length:

  • Line 34: Replaces [1,3) with '12' when len >= 3
  • Line 36: Replaces with 'R' otherwise

This could make debugging test failures more difficult if the text state isn't as expected.


121-122: Non-English comment detected - consider using English for broader team collaboration.

Line 121 contains a Korean comment. While the meaning is clear from context ("save text snapshots"), consider using English for consistency and team-wide accessibility.

-          // 텍스트 스냅샷 저장
+          // Save text snapshots

150-196: Consider adding error scenarios to multi-client tests.

The multi-client test suite thoroughly covers successful convergence scenarios. Consider adding test cases for:

  • Network failures during sync
  • Conflicting operations that might cause divergence
  • Undo operations when sync fails

Would you like me to help create additional test cases for error scenarios in multi-client undo operations?

packages/sdk/src/document/operation/edit_operation.ts (1)

170-174: Remove unnecessary type assertion.

Line 173 uses as any which bypasses TypeScript's type checking.

-        restoredAttrs = Array.from(Object.entries(attrsObj as any));
+        restoredAttrs = Array.from(Object.entries(attrsObj));

The getAttributes() method already returns Record<string, string>, so the type assertion is unnecessary.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b11b4a0 and 6cc2296.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • examples/vanilla-codemirror6/package.json (1 hunks)
  • examples/vanilla-codemirror6/src/main.ts (4 hunks)
  • packages/sdk/index.html (3 hunks)
  • packages/sdk/src/document/crdt/rga_tree_split.ts (3 hunks)
  • packages/sdk/src/document/crdt/text.ts (4 hunks)
  • packages/sdk/src/document/document.ts (4 hunks)
  • packages/sdk/src/document/history.ts (2 hunks)
  • packages/sdk/src/document/operation/edit_operation.ts (6 hunks)
  • packages/sdk/test/integration/history_text_test.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/sdk/test/integration/history_text_test.ts (1)
packages/sdk/test/integration/integration_helper.ts (1)
  • withTwoClientsAndDocuments (36-68)
packages/sdk/src/document/history.ts (1)
packages/sdk/src/document/operation/edit_operation.ts (1)
  • EditOperation (35-366)
packages/sdk/src/document/crdt/text.ts (3)
packages/sdk/src/document/crdt/gc.ts (1)
  • GCPair (24-27)
packages/sdk/src/api/yorkie/v1/resources_pb.ts (1)
  • DataSize (2926-2964)
packages/sdk/src/util/resource.ts (1)
  • DataSize (27-37)
packages/sdk/src/document/operation/edit_operation.ts (3)
packages/sdk/src/document/crdt/text.ts (2)
  • CRDTTextValue (73-207)
  • CRDTText (213-543)
packages/sdk/src/document/crdt/root.ts (1)
  • CRDTRoot (78-398)
packages/sdk/src/util/error.ts (1)
  • YorkieError (85-96)
packages/sdk/src/document/document.ts (2)
packages/sdk/src/document/operation/array_set_operation.ts (1)
  • ArraySetOperation (30-147)
packages/sdk/src/document/operation/edit_operation.ts (1)
  • EditOperation (35-366)
🔇 Additional comments (13)
examples/vanilla-codemirror6/package.json (1)

16-16: LGTM! Required dependency for the undo/redo keymap implementation.

The addition of @codemirror/view dependency is necessary for the CodeMirror keymap functionality used in the main.ts file to intercept undo/redo operations.

examples/vanilla-codemirror6/src/main.ts (1)

78-89: Good implementation of undo/redo event handling.

The subscription correctly handles both remote changes and undo/redo-driven local changes by checking for event.source === 'undoredo'. This ensures proper synchronization of undo/redo operations across clients.

packages/sdk/src/document/crdt/rga_tree_split.ts (3)

591-604: LGTM! Clean implementation of removed values tracking.

The addition of the fifth return element (removedValues) to track removed text values is well-implemented and properly integrated with the undo functionality.


734-783: Well-documented position normalization method.

The normalizePos method is thoroughly documented with clear examples and invariants. The implementation correctly traverses the physical chain to compute absolute offsets.


785-827: Verify refinePos overflow handling with concurrent edits

refinePos uses node.getContentLength() for the start node but node.getLength() for subsequent nodes; this asymmetry can leave a position anchored inside a removed node instead of advancing past it. There are no tests covering this.

  • Confirm intended semantics: should the initial node treat removed length as 0 (use getLength) or preserve the original rel (getContentLength)?
  • Add unit tests for: (a) pos anchored in a removed node, (b) pos crossing split boundaries under concurrent splits/deletes/inserts, (c) the undo path where edit_operation.ts calls refinePos (packages/sdk/src/document/operation/edit_operation.ts ~lines 100–110).
  • If removed nodes must be treated as length 0, change the initial partLen from getContentLength() to getLength() and update tests.
packages/sdk/index.html (2)

451-451: Good implementation of unified change handling.

The condition correctly handles both remote changes and undo/redo operations by checking event.source === 'undoredo', ensuring proper synchronization.


472-477: LGTM! Proper filtering of undo/redo origins.

The beforeChange handler correctly ignores undo/redo operations originating from CodeMirror's native commands, preventing conflicts with Yorkie's history management.

packages/sdk/test/integration/history_text_test.ts (2)

1-3: LGTM!

Imports are well organized and correctly reference the SDK and test helper modules.


81-102: Good test structure for single operations.

The single operation test suite provides good basic coverage of undo functionality. The consistent pattern of init → apply → undo → assert makes the tests easy to understand.

packages/sdk/src/document/crdt/text.ts (2)

237-275: Good implementation of extended edit return signature.

The changes correctly handle the new 5-tuple return value from rgaTreeSplit.edit, extracting and passing through the removedValues for undo support.


419-431: Well-structured delegation methods for position handling.

The refinePos and normalizePos methods appropriately delegate to the underlying rgaTreeSplit, maintaining separation of concerns.

packages/sdk/src/document/document.ts (1)

1527-1542: LGTM! Proper reconciliation of operations during change application.

The implementation correctly reconciles both ArraySetOperation and EditOperation when applying changes, ensuring the history stack remains consistent with document state.

packages/sdk/src/document/operation/edit_operation.ts (1)

158-194: Solid implementation of reverse operation generation.

The toReverseOperation method correctly:

  1. Concatenates removed content
  2. Preserves attributes when exactly one value was removed
  3. Calculates appropriate position range for the reverse operation
  4. Sets the isUndoOp flag to true for proper undo behavior

One minor suggestion: Consider adding a comment explaining why attributes are only restored when exactly one value is removed (lines 169-175).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
packages/sdk/src/document/operation/edit_operation.ts (5)

40-40: Avoid optional boolean for internal flag.

Prefer a concrete boolean default to reduce tri‑state logic.

Apply this diff:

-  private isUndoOp: boolean | undefined;
+  private isUndoOp: boolean;

And initialize it in the ctor with a default (see next comment).


155-187: Use returned start/end for reverse op; tighten attrs extraction; skip no-op reverse.

  • Accept normalized start/end to avoid computing end from content length.
  • Remove unnecessary any cast.
  • Optionally skip creating a no-op reverse when nothing changed.

Apply this diff:

-  private toReverseOperation(
-    removedValues: Array<CRDTTextValue>,
-    normalizedPos: RGATreeSplitPos,
-  ): Operation | undefined {
+  private toReverseOperation(
+    removedValues: Array<CRDTTextValue>,
+    normalizedStart: RGATreeSplitPos,
+    normalizedEnd: RGATreeSplitPos,
+  ): Operation | undefined {
     // 1) Content
     const restoredContent =
       removedValues && removedValues.length !== 0
         ? removedValues.map((v) => v.getContent()).join('')
         : '';
 
     // 2) Attribute
     let restoredAttrs: Array<[string, string]> | undefined;
     if (removedValues.length === 1) {
       const attrsObj = removedValues[0].getAttributes();
       if (attrsObj) {
-        restoredAttrs = Array.from(Object.entries(attrsObj as any));
+        restoredAttrs = Object.entries(attrsObj);
       }
     }
 
+    // Optional: if both restoredContent is empty and the original insert length is zero,
+    // skip creating a reverse op.
+    if (!restoredContent && normalizedStart.getRelativeOffset() === normalizedEnd.getRelativeOffset()) {
+      return undefined;
+    }
+
     // 3) Create Reverse Operation
     return EditOperation.create(
       this.getParentCreatedAt(),
-      normalizedPos,
-      RGATreeSplitPos.of(
-        normalizedPos.getID(),
-        normalizedPos.getRelativeOffset() + (this.content?.length ?? 0),
-      ),
+      normalizedStart,
+      normalizedEnd,
       restoredContent,
       restoredAttrs ? new Map(restoredAttrs) : new Map(),
       undefined,
       true,
     );
   }

204-223: Minor: generic on normalizePos is unused noise.

You can drop the generic parameter to simplify the signature; the method doesn’t depend on A.

Possible tweak:

-  public normalizePos<A extends Indexable>(root: CRDTRoot): [number, number] {
+  public normalizePos(root: CRDTRoot): [number, number] {

243-256: Validate contentLen and sanitize inputs.

Guard against non-integer/negative contentLen and equal ranges early.

Apply this diff:

   public reconcileOperation(
     rangeFrom: number,
     rangeTo: number,
     contentLen: number,
   ): void {
     if (!this.isUndoOp) {
       return;
     }
-    if (!Number.isInteger(rangeFrom) || !Number.isInteger(rangeTo)) {
+    if (!Number.isInteger(rangeFrom) || !Number.isInteger(rangeTo) || !Number.isInteger(contentLen) || contentLen < 0) {
       return;
     }
     if (rangeFrom > rangeTo) {
       return;
     }

262-265: Ensure fromPos ≤ toPos after apply().

Clamping both to ≥ 0 may still yield from > to; keep them ordered.

Apply this diff:

-    const apply = (na: number, nb: number) => {
-      this.fromPos = RGATreeSplitPos.of(this.fromPos.getID(), Math.max(0, na));
-      this.toPos = RGATreeSplitPos.of(this.toPos.getID(), Math.max(0, nb));
-    };
+    const apply = (na: number, nb: number) => {
+      const from = Math.max(0, na);
+      const to = Math.max(from, nb);
+      this.fromPos = RGATreeSplitPos.of(this.fromPos.getID(), from);
+      this.toPos = RGATreeSplitPos.of(this.toPos.getID(), to);
+    };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6cc2296 and dc26454.

📒 Files selected for processing (4)
  • packages/sdk/index.html (3 hunks)
  • packages/sdk/src/document/document.ts (2 hunks)
  • packages/sdk/src/document/history.ts (2 hunks)
  • packages/sdk/src/document/operation/edit_operation.ts (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/sdk/src/document/history.ts
  • packages/sdk/src/document/document.ts
  • packages/sdk/index.html
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KMSstudio
PR: yorkie-team/yorkie-js-sdk#1090
File: packages/sdk/src/document/operation/edit_operation.ts:250-319
Timestamp: 2025-09-16T08:12:44.626Z
Learning: KMSstudio prefers direct, straightforward implementations over abstracted helper methods and complex refactoring patterns, especially for domain-specific logic in the reconcileOperation method. They value simplicity and believe some logic should remain explicit rather than being over-engineered.
📚 Learning: 2025-09-16T08:12:44.626Z
Learnt from: KMSstudio
PR: yorkie-team/yorkie-js-sdk#1090
File: packages/sdk/src/document/operation/edit_operation.ts:250-319
Timestamp: 2025-09-16T08:12:44.626Z
Learning: KMSstudio prefers direct, straightforward implementations over abstracted helper methods and complex refactoring patterns, especially for domain-specific logic in the reconcileOperation method. They value simplicity and believe some logic should remain explicit rather than being over-engineered.

Applied to files:

  • packages/sdk/src/document/operation/edit_operation.ts
🧬 Code graph analysis (1)
packages/sdk/src/document/operation/edit_operation.ts (4)
packages/sdk/src/document/crdt/rga_tree_split.ts (1)
  • RGATreeSplitPos (167-246)
packages/sdk/src/document/crdt/text.ts (2)
  • CRDTTextValue (73-207)
  • CRDTText (213-543)
packages/sdk/src/document/document.ts (1)
  • Indexable (432-432)
packages/sdk/src/util/error.ts (1)
  • YorkieError (85-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build (22.x)
🔇 Additional comments (5)
packages/sdk/src/document/operation/edit_operation.ts (5)

21-21: Import looks correct and necessary.

Bringing in CRDTTextValue is appropriate for reverse-op construction.


105-108: Refining positions only for undo is sensible.

This avoids unnecessary churn for normal edits while ensuring stability for undo.


286-295: Overlap formulas: confirm intent re: inserted content.

In the start/end overlap cases you ignore contentLen in the new endpoints. If the intent is to exclude newly inserted external content from the undo range, this is fine; if not, b may need +contentLen (start overlap) or a may need +contentLen (end overlap).

Please verify with tests for these scenarios (absolute offsets):

  • rangeFrom < a < rangeTo < b with contentLen ∈ {0, 3, 10}
  • a < rangeFrom < b < rangeTo with contentLen ∈ {0, 3, 10}

If needed, here’s a focused Jest skeleton you can adapt:

test.each`
  a   | b   | rf  | rt  | ins | expA | expB
  ${5}| ${15}| ${2}| ${8}| ${0}| ${2} | ${9}
  ${5}| ${15}| ${2}| ${8}| ${4}| ${?} | ${?}
`('reconcileOperation overlap-start a=$a b=$b with ins=$ins', ({a,b,rf,rt,ins,expA,expB}) => {
  // construct op with fromPos=a, toPos=b; call reconcileOperation(rf, rt, ins); assert new from/to
});

48-57: Keep executedAt optional; default isUndoOp to false

Operation.getExecutedAt() already enforces presence (throws ErrNotReady) and toReverseOperation intentionally creates reverse EditOperation with executedAt === undefined, so making executedAt required would break that flow. Change only the isUndoOp handling to a safe default: in packages/sdk/src/document/operation/edit_operation.ts set this.isUndoOp = isUndoOp ?? false (or make the constructor param isUndoOp: boolean = false).

Likely an incorrect or invalid review comment.


137-138: Confirm reverseOp handling — ExecutionResult includes reverseOp?: Operation but callers are inconsistent.

Findings: ExecutionResult is defined with reverseOp?: Operation (packages/sdk/src/document/operation/operation.ts:191–195). Many operation.execute implementations return { opInfos, reverseOp } (e.g. edit_operation.ts). change.execute collects reverseOps and returns them (packages/sdk/src/document/change/change.ts:163–180). However some callers ignore reverseOps — e.g. packages/sdk/src/document/document.ts:1517 destructures only { opInfos } while other call sites use { opInfos, reverseOps } (document.ts:694, 2016).

Action: Confirm whether callers that drop reverseOps (document.ts:1517 and any others) should instead receive/propagate reverseOps; if dropping is intentional, no change needed.

Comment on lines +109 to 121
const [changes, pairs, diff, , removed] = text.edit(
[this.fromPos, this.toPos],
this.content,
this.getExecutedAt(),
Object.fromEntries(this.attributes),
versionVector,
);

root.acc(diff);
const reverseOp = this.toReverseOperation(
removed,
text.normalizePos(this.fromPos),
);

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Build reverse range from edit()’s returned posRange to avoid length/normalization drift.

Using this.content.length to derive the reverse end can be wrong with surrogate pairs, normalization, or internal splits. edit() already returns the precise RGATreeSplitPosRange; use that.

Apply this diff:

-    const [changes, pairs, diff, , removed] = text.edit(
+    const [changes, pairs, diff, posRange, removed] = text.edit(
       [this.fromPos, this.toPos],
       this.content,
       this.getExecutedAt(),
       Object.fromEntries(this.attributes),
       versionVector,
     );
-
-    const reverseOp = this.toReverseOperation(
-      removed,
-      text.normalizePos(this.fromPos),
-    );
+    const [startPos, endPos] = posRange;
+    const reverseOp = this.toReverseOperation(
+      removed,
+      text.normalizePos(startPos),
+      text.normalizePos(endPos),
+    );

And update toReverseOperation’s signature accordingly (see below).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [changes, pairs, diff, , removed] = text.edit(
[this.fromPos, this.toPos],
this.content,
this.getExecutedAt(),
Object.fromEntries(this.attributes),
versionVector,
);
root.acc(diff);
const reverseOp = this.toReverseOperation(
removed,
text.normalizePos(this.fromPos),
);
const [changes, pairs, diff, posRange, removed] = text.edit(
[this.fromPos, this.toPos],
this.content,
this.getExecutedAt(),
Object.fromEntries(this.attributes),
versionVector,
);
const [startPos, endPos] = posRange;
const reverseOp = this.toReverseOperation(
removed,
text.normalizePos(startPos),
text.normalizePos(endPos),
);
🤖 Prompt for AI Agents
In packages/sdk/src/document/operation/edit_operation.ts around lines 109 to
121, the reverse range is currently derived from this.content.length and
normalized positions which can drift for surrogate pairs, normalization, or
internal splits; instead, use the posRange returned by text.edit (the precise
RGATreeSplitPosRange) when constructing the reverse operation and change
toReverseOperation to accept that posRange directly; update the call to pass the
returned posRange and modify toReverseOperation’s signature/implementation to
consume the RGATreeSplitPosRange rather than computing an end from
this.content.length or re-normalizing positions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants