Text undo/redo #1126
Text undo/redo #1126
Conversation
WalkthroughAdds multi-user undo/redo: CRDT edits now return removed values; new position normalization/refinement APIs; EditOperation becomes undo-aware with reverse-op generation and reconciliation; History gains reconciliation/clear methods; Document integrates history reconciliation/clearing; CodeMirror example and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Client/Editor
participant Doc as Document
participant CRDT as CRDT (CRDTText / RGATreeSplit)
participant Hist as History
participant Remote as Remote/Network
Editor->>Doc: apply local Change (EditOperation)
Doc->>CRDT: CRDTText.edit(...) → changes + removedValues
CRDT-->>Doc: changes, removedValues
Doc->>Doc: Change.execute() → operations[], opInfos, reverseOps
Doc->>Hist: push operation(s) to undo stack
Doc->>Remote: publish operations
Remote-->>Doc: receive remote EditOperation
Doc->>Hist: reconcileTextEdit(parentCreatedAt, rangeFrom, rangeTo, contentLen)
Hist-->>Doc: undo/redo entries reconciled
Doc->>CRDT: apply remote edit (normalize/refine positions if undo)
CRDT-->>Doc: applied changes
Doc->>Editor: notify sync (syncText / handleOperations)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/sdk/src/document/crdt/rga_tree_split.ts (3)
⏰ 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)
🔇 Additional comments (2)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1126 +/- ##
==========================================
+ Coverage 80.92% 80.97% +0.05%
==========================================
Files 70 72 +2
Lines 5682 5899 +217
Branches 1172 1233 +61
==========================================
+ Hits 4598 4777 +179
- Misses 719 738 +19
- Partials 365 384 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
If the review progresses and there is further development, I plan to add a design guideline later as well. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/sdk/test/integration/history_text_test.ts (4)
9-50: Redundant length calculation pattern.The fallback
t.length ?? t.toString().lengthappears in every case. SinceText.lengthshould always be defined (per the CRDT implementation), the fallback seems unnecessary and adds noise.Consider simplifying to just
t.length:case 'insert': { - const len = t.length ?? t.toString().length; + const len = t.length; t.edit(len, len, 'X'); break; }Apply similar changes to other cases if
t.lengthis reliably defined.
52-74: Missingstylecase handler inapplyTextOp2.Unlike
applyTextOp1, this function lacks astylecase. Ifstyleis passed, the switch will silently do nothing. Consider adding a case or a default to throw for unhandled operations to avoid silent failures when tests are extended.case 'replace': { const len = t.length ?? t.toString().length; if (len > 0) t.edit(0, 1, 'Z'); else t.edit(0, 0, 'Z'); break; } + default: + throw new Error(`Unhandled operation: ${op}`); }
142-142: Non-English comment should be translated.The comment
// 텍스트 스냅샷 저장(Korean for "save text snapshot") should be in English for consistency with the rest of the codebase.- // 텍스트 스냅샷 저장 + // Save text snapshots const S: Array<string> = [];
176-216: Multi-client convergence test looks good but consider adding redo verification.The test validates document convergence after operations and undos, but doesn't verify that redo also converges correctly. Since undo/redo is the feature being tested, adding a redo step would provide more complete coverage.
Consider adding redo verification:
assert.equal( d1.toSortedJSON(), d2.toSortedJSON(), 'Mismatch after both undos', ); + + d1.history.redo(); + d2.history.redo(); + + await c1.sync(); + await c2.sync(); + await c1.sync(); + + assert.equal( + d1.toSortedJSON(), + d2.toSortedJSON(), + 'Mismatch after both redos', + ); }, task.name);examples/vanilla-codemirror6/src/main.ts (1)
51-58: Whitespace on line 51 and subscription condition logic.The condition
event.type === 'remote-change' || event.source === 'undoredo'correctly handles both remote changes and undo/redo events. However, line 51 has trailing whitespace that should be cleaned up.- + doc.subscribe('$.content', (event) => {packages/sdk/src/document/crdt/text.ts (1)
398-410: New position utility methods are clean delegations.Both
refinePosandnormalizePosproperly delegate to the underlyingrgaTreeSplitimplementation. The JSDoc comments are concise but could benefit from brief descriptions of what "refine" vs "normalize" mean semantically.Consider enhancing documentation:
/** - * `refinePos` refines the given RGATreeSplitPos. + * `refinePos` refines the given position by adjusting it to account for + * nodes that may have been split or removed since the position was created. */ public refinePos(pos: RGATreeSplitPos): RGATreeSplitPos { return this.rgaTreeSplit.refinePos(pos); }packages/sdk/src/document/operation/edit_operation.ts (2)
177-202: normalizePos method duplicates parent object lookup pattern.The lookup and validation logic (lines 181-195) is nearly identical to the pattern in
execute. Consider extracting a private helper method to reduce duplication.+ private getTextFromRoot<A extends Indexable>(root: CRDTRoot): CRDTText<A> { + const parentObject = root.findByCreatedAt(this.getParentCreatedAt()); + if (!parentObject) { + throw new YorkieError( + Code.ErrInvalidArgument, + `fail to find ${this.getParentCreatedAt()}`, + ); + } + if (!(parentObject instanceof CRDTText)) { + throw new YorkieError( + Code.ErrInvalidArgument, + `only Text operations are supported`, + ); + } + return parentObject as CRDTText<A>; + }
198-201: normalizePos returns relative offsets, not absolute indices.The method returns
[rangeFrom, rangeTo]where both values aregetRelativeOffset()from the normalized positions. The method name suggests it normalizes positions, but it actually returns relative offsets. This could be confusing for callers. Consider renaming togetNormalizedOffsetsor updating the JSDoc to clarify./** - * `normalizePos` normalizes the position of the edit operation. + * `normalizePos` returns the relative offsets of the normalized from/to positions. + * @returns A tuple [fromOffset, toOffset] representing the relative offsets. */ public normalizePos<A extends Indexable>(root: CRDTRoot): [number, number] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
examples/vanilla-codemirror6/package.json(1 hunks)examples/vanilla-codemirror6/src/main.ts(5 hunks)packages/sdk/src/document/change/change.ts(3 hunks)packages/sdk/src/document/crdt/rga_tree_split.ts(3 hunks)packages/sdk/src/document/crdt/text.ts(5 hunks)packages/sdk/src/document/document.ts(4 hunks)packages/sdk/src/document/history.ts(3 hunks)packages/sdk/src/document/operation/edit_operation.ts(6 hunks)packages/sdk/test/integration/history_text_test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-01T04:32:18.880Z
Learnt from: emplam27
Repo: yorkie-team/yorkie-js-sdk PR: 1037
File: packages/sdk/src/document/json/array.ts:574-598
Timestamp: 2025-08-01T04:32:18.880Z
Learning: ArrayProxy.insertAfterInternal method returns CRDT elements, not target CRDT arrays, in packages/sdk/src/document/json/array.ts.
Applied to files:
packages/sdk/src/document/crdt/rga_tree_split.tspackages/sdk/src/document/crdt/text.ts
📚 Learning: 2025-03-17T07:16:43.740Z
Learnt from: hackerwins
Repo: yorkie-team/yorkie-js-sdk PR: 954
File: examples/react-flow/src/main.tsx:44-59
Timestamp: 2025-03-17T07:16:43.740Z
Learning: In the react-flow example for yorkie-js-sdk, using a date-based document key (`react-flow-YYYYMMDD`) is an intentional design pattern to create a new document each day.
Applied to files:
examples/vanilla-codemirror6/src/main.ts
🧬 Code graph analysis (6)
packages/sdk/test/integration/history_text_test.ts (2)
packages/sdk/src/document/document.ts (1)
Document(512-1970)packages/sdk/test/integration/integration_helper.ts (1)
withTwoClientsAndDocuments(37-69)
packages/sdk/src/document/history.ts (1)
packages/sdk/src/document/operation/edit_operation.ts (1)
EditOperation(35-308)
packages/sdk/src/document/crdt/rga_tree_split.ts (3)
packages/sdk/src/document/crdt/gc.ts (1)
GCPair(24-27)packages/sdk/src/util/resource.ts (1)
DataSize(27-37)packages/sdk/src/util/error.ts (1)
YorkieError(85-96)
examples/vanilla-codemirror6/src/main.ts (2)
examples/profile-stack/main.js (1)
doc(9-11)examples/vanilla-codemirror6/src/type.ts (2)
YorkieDoc(3-5)YorkiePresence(7-10)
packages/sdk/src/document/operation/edit_operation.ts (8)
packages/sdk/src/yorkie.ts (2)
TimeTicket(70-70)Indexable(53-53)packages/sdk/src/document/time/ticket.ts (1)
TimeTicket(51-186)packages/sdk/public/devtool/text.js (1)
fromPos(666-666)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-554)packages/sdk/src/document/document.ts (1)
Indexable(392-392)packages/sdk/src/document/crdt/root.ts (1)
CRDTRoot(78-411)packages/sdk/src/util/error.ts (1)
YorkieError(85-96)
packages/sdk/src/document/crdt/text.ts (4)
packages/sdk/src/document/crdt/gc.ts (1)
GCPair(24-27)packages/sdk/src/api/yorkie/v1/resources_pb.ts (1)
DataSize(3151-3189)packages/sdk/src/util/resource.ts (1)
DataSize(27-37)packages/sdk/src/document/crdt/rga_tree_split.ts (2)
RGATreeSplitPosRange(248-248)RGATreeSplitPos(167-246)
🔇 Additional comments (24)
examples/vanilla-codemirror6/package.json (1)
16-16: LGTM!The addition of
@codemirror/viewdependency is appropriate for enabling CodeMirror 6 view-layer features needed for undo/redo keybindings.packages/sdk/src/document/crdt/rga_tree_split.ts (2)
607-674: LGTM!The extended return type now includes
removedValuesas a fifth element, correctly collected from removed nodes. This enables the undo/redo feature to restore previously removed text content.
798-806: LGTM!The
normalizePosmethod correctly converts a local position to an absolute offset measured from the head, which is essential for position reconciliation during undo/redo operations.packages/sdk/src/document/history.ts (2)
102-107: LGTM!The
clearUndomethod follows the same pattern asclearRedoand correctly resets the undo stack.
161-186: LGTM!The
reconcileTextEditmethod correctly scans both undo and redo stacks, identifies matchingEditOperationinstances byparentCreatedAt, and reconciles their positions. This ensures undo/redo operations remain consistent when concurrent text edits occur.packages/sdk/src/document/document.ts (3)
1332-1333: LGTM!Clearing history after applying the snapshot and re-applying local changes is correct. The old undo/redo operations would reference stale positions and nodes that may no longer exist in the new document state.
1349-1352: LGTM!The
clearHistoryhelper method cleanly encapsulates clearing both undo and redo stacks.
1447-1463: LGTM!The integration correctly reconciles undo/redo history when remote text edits are applied. By iterating over actually executed operations and reconciling positions for
EditOperationinstances, this ensures that pending undo/redo operations remain valid after concurrent edits from other clients.packages/sdk/src/document/change/change.ts (1)
153-196: LGTM!The enhanced
executemethod now returns the list of successfully executed operations alongsideopInfosandreverseOps. This enables the document layer to reconcile text edit positions based on what was actually applied, which is essential for maintaining undo/redo consistency across concurrent edits.packages/sdk/test/integration/history_text_test.ts (5)
1-7: LGTM: Clean imports and clear type definition.The
TextOptype andopsarray are well-defined. The TODO comment appropriately tracks pending work for style operations and multi-client tests.
125-169: Well-structured chained operations test with comprehensive coverage.The triple-nested loop effectively covers all 27 combinations of
insert,delete,replaceoperations. The snapshot-based verification approach is sound for validating step-by-step undo correctness.
218-261: Overlapping edits test provides good edge case coverage.The test correctly sets up overlapping edit ranges
[2,6)and[4,9)and validates convergence after both operations and undos. This is a critical scenario for CRDT undo/redo.
263-308: Containing edits test covers important nested edit scenario.The outer
[2,9)and inner[4,6)edit ranges test the containment case well. The undo ordering (d2 then d1) is intentionally different from the edit ordering, which tests robustness.
310-365: Style test is appropriately skipped pending implementation.The skipped test provides a clear template for future style undo/redo testing, which aligns with the PR scope (edit only, not style).
examples/vanilla-codemirror6/src/main.ts (2)
146-170: Well-structured syncText function with proper cursor restoration.The function correctly handles full text replacement with cursor position preservation via the presence selection. The snapshot subscription ensures re-sync when document state is replaced.
63-70: Correct removal of undo/redo from tracked user events.Since undo/redo is now handled via custom keybindings that directly interact with
doc.history, removing them from the tracked events array prevents duplicate handling.packages/sdk/src/document/crdt/text.ts (3)
243-249: Extended edit return type properly documented.The 5-tuple return type
[Array<TextChange<A>>, Array<GCPair>, DataSize, RGATreeSplitPosRange, Array<CRDTTextValue>]correctly adds removed values as the final element. This maintains backward compatibility since existing code can ignore the new element.
257-274: Edit method correctly propagates removed values from RGATreeSplit.The destructuring captures
removedValuesfrom the underlying edit call and passes it through in the return tuple. This enables undo operations to reconstruct deleted content.
527-535: New posToIndex method with preferToLeft parameter.The method correctly delegates to
rgaTreeSplit.posToIndexwith the optionalpreferToLeftparameter. The underlying RGATreeSplit implementation accepts bothposandpreferToLeftparameters and uses them appropriately for cursor positioning during undo/redo operations.packages/sdk/src/document/operation/edit_operation.ts (5)
40-56: Optional isUndoOp flag properly integrated into constructor.The new
isUndoOpfield is correctly added as an optional parameter to maintain backward compatibility. The field is assigned in the constructor body.
105-109: Position refinement for undo operations is correctly placed.Refining positions before the edit ensures that the undo operation targets the correct location even if the text structure has changed since the original operation. The refinement only occurs for undo operations, avoiding overhead for normal edits.
207-261: reconcileOperation conditions serve valid purposes and are not redundant.Analysis of the flagged conditions reveals they are purposeful, not redundant:
Line 241
rangeFrom !== rangeTo: This excludes zero-length ranges from the "fully overlap: contains" case. An empty range cannot meaningfully contain another range, so the guard is necessary.Line 245
a !== b: This ensures the operation spans a non-zero range before applying the reconciliation logic. A single-position operation (wherea === b) should not be reconciled using this branch.The boundary equality cases are intentionally handled by strict inequality checks (
<,>vs<=,>=), which systematically partition overlap scenarios without double-handling edge cases. However, adding unit tests for boundary conditions would improve confidence in the logic, as no existing tests were found forreconcileOperation.
166-169: Remove this review concern — the code handles all cases correctly.The
contentproperty is strictly typed asstring(neverundefined), so the optional chaining and nullish coalescing are unnecessary but harmless. For pure deletions wherecontentis an empty string'', the calculation correctly yieldsnormalizedFromPos.offset + 0, which is the correct range for the reverse operation. The reverse operation logic is sound: it deletes what was inserted (this.content.length) and restores what was deleted (restoredContent). Extensive test coverage with pure deletion cases (40+ instances across test files) confirms this works as intended.
226-229: No action needed — mutation offromPosandtoPosis intentional.The
reconcilemethod is part of the abstractOperationinterface contract and is specifically designed to update operation state based on transformation deltas. Creating newRGATreeSplitPosinstances and assigning them tothis.fromPosandthis.toPosis the intended behavior, not a side effect. There is no evidence in the codebase of operations being reused after reconciliation or of this pattern causing issues.
| public refinePos(pos: RGATreeSplitPos): RGATreeSplitPos { | ||
| let node = this.findFloorNode(pos.getID()); | ||
| if (!node) { | ||
| throw new YorkieError( | ||
| Code.ErrInvalidArgument, | ||
| `the node of the given id should be found: ${pos.getID().toTestString()}`, | ||
| ); | ||
| } | ||
|
|
||
| let offsetInPart = pos.getRelativeOffset(); | ||
| let partLen = node.getContentLength(); | ||
|
|
||
| while (offsetInPart > partLen) { | ||
| offsetInPart -= partLen; | ||
| const next: RGATreeSplitNode<T> | undefined = node!.getNext(); | ||
|
|
||
| if (!next) { | ||
| return RGATreeSplitPos.of(node.getID(), partLen); | ||
| } | ||
|
|
||
| node = next; | ||
| partLen = node.getLength(); | ||
| } | ||
|
|
||
| return RGATreeSplitPos.of(node.getID(), offsetInPart); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for tests that verify refinePos behavior
rg -n "refinePos" --type=ts -A 5 -B 2Repository: yorkie-team/yorkie-js-sdk
Length of output: 3764
🏁 Script executed:
# Search for getContentLength and getLength definitions
rg -n "getContentLength\(\)|getLength\(\)" packages/sdk/src/document/crdt/rga_tree_split.ts --type=ts -B 2 -A 3Repository: yorkie-team/yorkie-js-sdk
Length of output: 1498
🏁 Script executed:
# Examine the RGATreeSplitNode class to understand these methods
ast-grep --pattern 'class RGATreeSplitNode {
$$$
}'Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
🏁 Script executed:
# Search for findFloorNode to understand if it can return removed nodes
rg -n "findFloorNode" packages/sdk/src/document/crdt/rga_tree_split.ts --type=ts -A 10 -B 2Repository: yorkie-team/yorkie-js-sdk
Length of output: 2734
🏁 Script executed:
# Search for test cases that test refinePos with removed content
rg -n "refinePos" --type=ts -l | xargs grep -l "remov\|delete" | head -5Repository: yorkie-team/yorkie-js-sdk
Length of output: 210
🏁 Script executed:
# Search specifically in test files for refinePos tests
fd ".*\.test\.ts$|.*\.spec\.ts$" --type=f | xargs rg -l "refinePos" 2>/dev/null | head -3Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
Fix inconsistent length calculation for initial node in refinePos.
Line 838 uses getContentLength() (ignores removal status) for the initial node, but line 849 uses getLength() (returns 0 for removed nodes) for subsequent nodes. This violates the documented behavior (line 812-813) that states: "Counts only live characters: removed nodes are treated as length 0."
When the initial node is removed, the current code incorrectly uses its full content length instead of treating it as 0. Change line 838 to use node.getLength() for consistency with the loop and the documented specification.
🤖 Prompt for AI Agents
In packages/sdk/src/document/crdt/rga_tree_split.ts around lines 828 to 853, the
initial node length is obtained with node.getContentLength() (line ~838) which
ignores removal status, causing removed nodes to be counted; change that call to
node.getLength() so the initial node is treated the same as subsequent nodes in
the loop, and keep the rest of the logic—assign partLen = node.getLength() and
let the while loop and return statements operate as before.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/vanilla-codemirror6/src/main.ts (1)
117-145: Remove debug logging and confirm keymap precedence over CodeMirror historyThe custom
cmUndoRedoKeymapcorrectly bindsMod-z/Mod-Shift-ztodoc.history.undo/redo, but the inlineconsole.log('undo')/console.log('redo')are debug artifacts and should be removed before merge. Also, sincebasicSetupincludes CodeMirror’s own history/keymap, it’s worth confirming via manual testing that your bindings reliably win for these keys (no double-undo or missed calls).Apply this minimal diff to drop the logs:
const cmUndoRedoKeymap = keymap.of([ { key: 'Mod-z', run: () => { - // To check undo works properly - console.log('undo'); if (doc.history.canUndo()) { doc.history.undo(); - } return true; }, }, { key: 'Mod-Shift-z', run: () => { - // To check redo works properly - console.log('redo'); if (doc.history.canRedo()) { doc.history.redo(); - } return true; }, }, ]);
🧹 Nitpick comments (1)
examples/vanilla-codemirror6/src/main.ts (1)
23-27: Dynamic fallbackdocKeyavoids accidental document sharingUsing a unique fallback key (
vanilla-codemirror6-${Date.now()}) means users without adockeyquery parameter no longer collide into the same document, while explicit sharing still works via the URL param. This matches the intent of avoiding unintended shared docs; choose this per-load uniqueness vs a per-day pattern based on what behavior you want long term. Based on learnings, this diverges from the date-based pattern used in the react-flow example but is still a reasonable choice here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/vanilla-codemirror6/src/main.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-17T07:16:43.740Z
Learnt from: hackerwins
Repo: yorkie-team/yorkie-js-sdk PR: 954
File: examples/react-flow/src/main.tsx:44-59
Timestamp: 2025-03-17T07:16:43.740Z
Learning: In the react-flow example for yorkie-js-sdk, using a date-based document key (`react-flow-YYYYMMDD`) is an intentional design pattern to create a new document each day.
Applied to files:
examples/vanilla-codemirror6/src/main.ts
🧬 Code graph analysis (1)
examples/vanilla-codemirror6/src/main.ts (1)
examples/vanilla-codemirror6/src/type.ts (2)
YorkieDoc(3-5)YorkiePresence(7-10)
⏰ 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 (2)
examples/vanilla-codemirror6/src/main.ts (2)
53-58: Undo/redo event routing through$.contentsubscription looks consistentRouting operations when
event.type === 'remote-change'orevent.source === 'undoredo'and then applying them viahandleOperationsis a clean way to unify remote edits and history-driven changes, and skipping transactions annotated withTransaction.remotein the update listener avoids feedback loops. Just ensure that'undoredo'matches the exact source string emitted by the history layer so local undo/redo always propagates correctly.Also applies to: 66-80
148-172:syncText+ snapshot handling cleanly resyncs the editor and restores selectionThe new
syncTexthelper that replaces the entire CM document fromroot.content.toString(), annotates the transaction as remote, and reconstructs the selection fromgetMyPresence().selectionprovides a robust way to handle snapshot events without corrupting local history or presence. This should keep the view aligned with Yorkie Text even after compaction/history operations.
hackerwins
force-pushed
the
main
branch
2 times, most recently
from
2e116ac to
560f07d
Compare
hackerwins
force-pushed
the
text-reverse
branch
from
145f0ea to
3bd7e20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
examples/vanilla-codemirror6/src/main.ts (1)
122-123: Remove debugconsole.logstatements before merging.The
console.log('undo')andconsole.log('redo')calls are debug artifacts that should be removed before this PR is merged.run: () => { - // To check undo works properly - console.log('undo'); if (doc.history.canUndo()) {run: () => { - // To check redo works properly - console.log('redo'); if (doc.history.canRedo()) {Also applies to: 134-135
packages/sdk/src/document/operation/edit_operation.ts (1)
143-175: Attributes restoration limited to single-segment deletions.As noted in a previous review, when
toReverseOperationreverses a deletion spanning multiple text segments with different attributes, only the first segment's attributes are preserved (line 156-161). Multi-segment deletions with heterogeneous attributes will restore text content but apply only the first segment's attributes to the entire result.This is a known limitation that should be documented or enhanced in a follow-up.
packages/sdk/src/document/crdt/rga_tree_split.ts (1)
828-853: Inconsistent length calculation for initial node inrefinePos.Line 838 uses
getContentLength()(ignores removal status) for the initial node, but line 849 usesgetLength()(returns 0 for removed nodes) for subsequent nodes. This violates the documented behavior (lines 812-813) stating: "Counts only live characters: removed nodes are treated as length 0."When the initial node is removed, the current code incorrectly uses its full content length instead of treating it as 0.
Apply this diff to fix the inconsistency:
let offsetInPart = pos.getRelativeOffset(); - let partLen = node.getContentLength(); + let partLen = node.getLength(); while (offsetInPart > partLen) {
🧹 Nitpick comments (2)
packages/sdk/test/integration/history_text_test.ts (2)
9-50: Consider adding JSDoc for helper functions.The static analysis tool flags missing JSDoc comments for
applyTextOp1andapplyTextOp2. While these are test helpers, adding brief documentation would improve readability. Also, consider whethert.length ?? t.toString().lengthfallback is necessary—ifTextalways exposes.length, the fallback may be redundant.+/** + * Applies a text operation to the document using position at end of text. + */ function applyTextOp1(doc: Document<{ t: Text }>, op: TextOp) {
142-143: Use English for code comments.The comment
텍스트 스냅샷 저장(Korean for "save text snapshot") should be in English for codebase consistency.- // 텍스트 스냅샷 저장 + // Save text snapshots const S: Array<string> = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
examples/vanilla-codemirror6/package.json(1 hunks)examples/vanilla-codemirror6/src/main.ts(5 hunks)packages/sdk/src/document/change/change.ts(3 hunks)packages/sdk/src/document/crdt/rga_tree_split.ts(3 hunks)packages/sdk/src/document/crdt/text.ts(5 hunks)packages/sdk/src/document/document.ts(4 hunks)packages/sdk/src/document/history.ts(3 hunks)packages/sdk/src/document/operation/edit_operation.ts(6 hunks)packages/sdk/test/integration/history_text_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/src/document/history.ts
- examples/vanilla-codemirror6/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-17T07:16:43.740Z
Learnt from: hackerwins
Repo: yorkie-team/yorkie-js-sdk PR: 954
File: examples/react-flow/src/main.tsx:44-59
Timestamp: 2025-03-17T07:16:43.740Z
Learning: In the react-flow example for yorkie-js-sdk, using a date-based document key (`react-flow-YYYYMMDD`) is an intentional design pattern to create a new document each day.
Applied to files:
examples/vanilla-codemirror6/src/main.ts
🧬 Code graph analysis (5)
packages/sdk/src/document/crdt/rga_tree_split.ts (2)
packages/sdk/src/util/resource.ts (1)
DataSize(27-37)packages/sdk/src/util/error.ts (1)
YorkieError(85-96)
packages/sdk/src/document/change/change.ts (3)
packages/sdk/src/api/yorkie/v1/resources_pb.ts (1)
Operation(199-264)packages/sdk/src/document/operation/operation.ts (1)
OpInfo(40-45)packages/sdk/src/document/history.ts (1)
HistoryOperation(28-33)
packages/sdk/test/integration/history_text_test.ts (2)
packages/sdk/src/document/document.ts (1)
Document(517-1976)packages/sdk/test/integration/integration_helper.ts (1)
withTwoClientsAndDocuments(37-69)
packages/sdk/src/document/document.ts (1)
packages/sdk/src/document/operation/edit_operation.ts (1)
EditOperation(35-308)
packages/sdk/src/document/crdt/text.ts (3)
packages/sdk/src/document/crdt/gc.ts (1)
GCPair(24-27)packages/sdk/src/util/resource.ts (1)
DataSize(27-37)packages/sdk/src/document/crdt/rga_tree_split.ts (2)
RGATreeSplitPosRange(248-248)RGATreeSplitPos(167-246)
🪛 ESLint
packages/sdk/test/integration/history_text_test.ts
[error] 9-10: Missing JSDoc comment.
(jsdoc/require-jsdoc)
[error] 52-53: Missing JSDoc comment.
(jsdoc/require-jsdoc)
⏰ 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 (25)
examples/vanilla-codemirror6/src/main.ts (3)
145-150: Good: Keymap ordering ensures custom undo/redo takes precedence.Placing
cmUndoRedoKeymapbeforebasicSetupcorrectly ensures the custom undo/redo handlers override the default CodeMirror history extension.
153-177: LGTM!The
syncTextfunction correctly handles snapshot synchronization by replacing editor content and restoring cursor position from presence. TheTransaction.remoteannotation properly prevents feedback loops.
187-200: The non-null assertion onop.value!.contentis safe. TheEditOpInfotype definesvalueas a required property, not optional, guaranteeing it is always present. No defensive check is needed.Likely an incorrect or invalid review comment.
packages/sdk/src/document/document.ts (3)
86-86: LGTM!The import is correctly added for the
EditOperationtype used in the reconciliation logic below.
1339-1339: LGTM!Clearing the history after applying a snapshot is the correct approach, as the undo/redo stacks would reference stale positions that no longer exist in the new document state.
1355-1358: LGTM!The
clearHistory()helper method correctly clears both undo and redo stacks. This provides a clean API for resetting history state.packages/sdk/test/integration/history_text_test.ts (4)
76-96: LGTM!The single-operation undo tests correctly verify that each operation (insert, delete, replace) can be undone to restore the original text content.
125-169: LGTM!The chained operations test comprehensively covers all 27 combinations (3³) of insert/delete/replace operations and verifies step-by-step undo correctness. Good use of snapshot array to track intermediate states.
171-216: LGTM!The multi-client convergence tests properly sync between clients and verify that both documents converge after operations and undos. The triple sync pattern (
c1.sync(),c2.sync(),c1.sync()) ensures full convergence.
218-261: LGTM!The overlapping edits test is an important edge case for CRDT text undo. The test correctly verifies convergence after concurrent overlapping edits are both undone.
packages/sdk/src/document/change/change.ts (3)
157-161: LGTM!The return type expansion to include
operations: Array<Operation>is well-designed. This enables callers to access the actual executed operations for reconciliation purposes while maintaining backward compatibility withopInfosandreverseOps.
163-177: LGTM!The implementation correctly collects only successfully executed operations (those that pass the
!executionResultcheck) intochangeOperations. This ensures that reconciliation only processes operations that actually modified the document state.
196-196: LGTM!The return statement correctly includes all three components needed by the caller.
packages/sdk/src/document/operation/edit_operation.ts (5)
21-21: LGTM!The
CRDTTextValueimport is correctly added to support thetoReverseOperationmethod which processes removed values.
40-56: LGTM!The
isUndoOpfield and constructor changes are clean. The field is optional and defaults toundefined, maintaining backward compatibility.
106-122: LGTM!The undo operation handling correctly refines positions before editing and generates the reverse operation using the normalized from position. The refinement is necessary because the original positions may reference split nodes that have been modified.
177-202: LGTM!The
normalizePosmethod correctly validates the parent object type and returns normalized offsets. The implementation properly handles type checking and error cases.
207-261: Consider adding inline comments explaining each overlap scenario for improved maintainability.The
reconcileOperationmethod correctly handles six distinct overlap cases when reconciling undo/redo edit operations:
- No overlap (rangeTo <= a): Shift both positions by net change
- No overlap (b <= rangeFrom): No adjustment needed
- Fully contained (rangeFrom <= a && b <= rangeTo): Collapse to rangeFrom
- Contains the range (a <= rangeFrom && rangeTo <= b): Adjust end by net change
- Overlap at start (rangeFrom < a && a < rangeTo && rangeTo < b): Adjust both positions
- Overlap at end (a < rangeFrom && rangeFrom < b && b < rangeTo): Adjust end to rangeFrom
The method includes proper input validation and the logic is sound. However, adding brief inline comments above each conditional block (especially for cases 3-6) would significantly improve code clarity for future maintainers, given the complexity of the position calculations and the lack of direct unit test documentation.
packages/sdk/src/document/crdt/rga_tree_split.ts (2)
600-674: LGTM!The expanded return type and
removedValuescollection are well-implemented. The removed values are correctly collected from the removed nodes, enabling the reverse operation to restore deleted content.
798-806: LGTM!The
normalizePosmethod correctly converts a local position to an absolute offset measured from the head of the chain. This enables position normalization for undo operations.packages/sdk/src/document/crdt/text.ts (5)
29-29: LGTM: Import addition supports new position methods.The addition of
RGATreeSplitPosto the imports is necessary for the new position manipulation methods added later in the file.
257-274: Implementation correctly returns removed values for undo/redo.The changes properly:
- Capture
removedValuesfrom the underlyingrgaTreeSplit.editcall- Map value changes to
TextChangeobjects with parsed attributes and content- Return the expanded 5-tuple including the removed values
Note: The 4th element
[caretPos, caretPos]creates a collapsed range (zero-length), which represents the cursor position after the edit.
405-410: New position normalization method added for undo/redo support.The
normalizePosmethod exposes position normalization at the CRDTText level, delegating to the underlying RGATreeSplit. This complementsrefinePosfor undo/redo position handling.Verify this method is used in the undo/redo implementation to ensure it's integrated as intended.
398-403: Position refinement method for undo/redo operations is actively used.The
refinePosmethod correctly exposes position refinement at the CRDTText API level and is used in the undo operation implementation (EditOperation.ts, lines 107-108) to refinefromPosandtoPoscoordinates. The underlyingRGATreeSplit.refinePoshandles position adjustments after node splits, which is essential for correctly replaying undo operations.
527-535: New position-to-index conversion method added to public CRDTText API.The
posToIndexmethod exposes position-to-index conversion as a public method in the CRDTText class, delegating to the underlying RGATreeSplit implementation. ThepreferToLeftparameter (defaulting tofalse) handles ambiguous position cases.The method is currently unused within the codebase. Verify that this is intentional (e.g., for future features or external API consumers).
| }); | ||
| await client.activate(); | ||
|
|
||
| const params = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
Remove unused params variable.
The params variable is created but never used in this file. This appears to be leftover code that should be cleaned up.
- const params = new URLSearchParams(window.location.search);📝 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.
| const params = new URLSearchParams(window.location.search); |
🤖 Prompt for AI Agents
In examples/vanilla-codemirror6/src/main.ts around line 23, remove the unused
variable declaration "const params = new
URLSearchParams(window.location.search);" since it's never referenced; simply
delete that line (or the declaration) to clean up leftover code and ensure there
are no linter warnings about unused variables.
| ): [ | ||
| Array<TextChange<A>>, | ||
| Array<GCPair>, | ||
| DataSize, | ||
| RGATreeSplitPosRange, | ||
| Array<CRDTTextValue>, | ||
| ] { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the edit method in the file
sed -n '240,260p' packages/sdk/src/document/crdt/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 748
🏁 Script executed:
# Search for all calls to .edit( in TypeScript files
rg -n '\.edit\(' packages/sdk/src --type=ts -B 2 -A 5Repository: yorkie-team/yorkie-js-sdk
Length of output: 4834
🏁 Script executed:
# Also search more broadly for potential edit method calls
rg -n 'edit\(' packages/sdk/src --type=ts | grep -v '//' | head -50Repository: yorkie-team/yorkie-js-sdk
Length of output: 1138
🏁 Script executed:
# Check the json/text.ts edit call more carefully
sed -n '110,125p' packages/sdk/src/document/json/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 528
🏁 Script executed:
# Check the CRDTTree.edit signature
sed -n '1103,1120p' packages/sdk/src/document/crdt/tree.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 674
🏁 Script executed:
# Check RGATreeSplit.edit signature
sed -n '602,630p' packages/sdk/src/document/crdt/rga_tree_split.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 788
🏁 Script executed:
# Check CRDTText.edit full signature and return type
sed -n '237,275p' packages/sdk/src/document/crdt/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 1215
🏁 Script executed:
# Check what json/text.ts does with the returned values from edit
sed -n '100,135p' packages/sdk/src/document/json/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 1039
🏁 Script executed:
# Check if there's any usage of removedValues that was expected to be captured
rg -n 'removedValues' packages/sdk/src/document/json/text.ts -B 5 -A 5Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
🏁 Script executed:
# Verify the json/text.ts class structure - does it use the 5th element?
sed -n '1,120p' packages/sdk/src/document/json/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 3869
🏁 Script executed:
# Double-check the exact position mapping - what is rangeAfterEdit getting assigned to
sed -n '116,125p' packages/sdk/src/document/json/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 253
🏁 Script executed:
# Check if removeValues should be used anywhere in the json/text.ts context
sed -n '88,140p' packages/sdk/src/document/json/text.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 1410
🏁 Script executed:
# Verify what the old return type was - check git history or any documentation
# Let's search for any comments or type definitions that might indicate the old signature
rg -n 'RGATreeSplitPosRange|removedValues' packages/sdk/src/document/crdt/text.ts -B 3 -A 1Repository: yorkie-team/yorkie-js-sdk
Length of output: 1400
🏁 Script executed:
# Check if there are any tests that show the expected behavior
fd -t f 'text.test.ts|text.spec.ts' packages/sdk --exec grep -l "edit" {} \;Repository: yorkie-team/yorkie-js-sdk
Length of output: 162
🏁 Script executed:
# Let's also check edit_operation to see how it uses the 5-tuple
sed -n '105,125p' packages/sdk/src/document/operation/edit_operation.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 583
Update all callers to destructure the expanded 5-tuple return type.
The CRDTText.edit() method now returns a 5-tuple instead of 4-tuple: [Array<TextChange<A>>, Array<GCPair>, DataSize, RGATreeSplitPosRange, Array<CRDTTextValue>]. The new 5th element contains removed values needed for undo/redo functionality.
Incomplete updates found:
packages/sdk/src/document/operation/edit_operation.ts:111correctly captures all 5 elements with[changes, pairs, diff, , removedValues]and usesremovedValuesfor reverse operationspackages/sdk/src/document/json/text.ts:116only destructures 4 elements[, pairs, diff, rangeAfterEdit], silently ignoring the 5th element (removed values)
Update json/text.ts line 116 to capture the removed values, or explicitly acknowledge if they're intentionally unused at that layer.
🤖 Prompt for AI Agents
In packages/sdk/src/document/json/text.ts around line 116, the call to
CRDTText.edit is still destructuring only 4 elements and ignoring the new 5th
return value (removed values); update the destructuring to capture the fifth
element (for example: [, pairs, diff, rangeAfterEdit, removedValues] or [,
pairs, diff, rangeAfterEdit, _removedValues] if intentionally unused) and, if
the removed values are relevant at this layer, thread them into the JSON/text
undo/redo handling; otherwise explicitly name the unused variable with a leading
underscore to document intentional omission.
| const { opInfos, operations } = change.execute( | ||
| this.root, | ||
| this.presences, | ||
| source, | ||
| ); | ||
| for (const op of operations) { | ||
| if (op instanceof EditOperation) { | ||
| const [from, to] = op.normalizePos(this.root); | ||
|
|
||
| this.internalHistory.reconcileTextEdit( | ||
| op.getParentCreatedAt(), | ||
| from, | ||
| to, | ||
| op.getContent()?.length ?? 0, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's test coverage for reconciliation when parent is removed
rg -n "reconcileTextEdit|normalizePos" --type ts packages/sdk/test -C 3 | head -100Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Find the normalizePos implementation
rg -n "normalizePos" --type ts packages/sdk/src -A 10 | head -80Repository: yorkie-team/yorkie-js-sdk
Length of output: 6249
🏁 Script executed:
#!/bin/bash
# Check the change.execute method and surrounding error handling
rg -n "execute\(" --type ts packages/sdk/src/document/document.ts -B 2 -A 15 | head -100Repository: yorkie-team/yorkie-js-sdk
Length of output: 2838
🏁 Script executed:
#!/bin/bash
# Look for error handling patterns around reconcileTextEdit
rg -n "reconcileTextEdit" --type ts packages/sdk/src -B 5 -A 10Repository: yorkie-team/yorkie-js-sdk
Length of output: 2581
🏁 Script executed:
#!/bin/bash
# Search for test files that might test the reconciliation scenario
find packages/sdk/test -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look for any tests related to removed or deleted text operations
rg -n "EditOperation|removeNode|remove.*parent" --type ts packages/sdk/test -l | head -10Repository: yorkie-team/yorkie-js-sdk
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check if there's any try-catch around change.execute or in the method containing the code
sed -n '1440,1480p' packages/sdk/src/document/document.tsRepository: yorkie-team/yorkie-js-sdk
Length of output: 1191
🏁 Script executed:
#!/bin/bash
# Look for broader error handling context in the notifyLocalChange method
rg -n "notifyLocalChange" --type ts packages/sdk/src/document/document.ts -A 50 | head -80Repository: yorkie-team/yorkie-js-sdk
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look at the Change.execute implementation to see if it validates parent existence
rg -n "class Change|execute\(" --type ts packages/sdk/src/document -A 5 | grep -A 20 "execute("Repository: yorkie-team/yorkie-js-sdk
Length of output: 9409
🏁 Script executed:
#!/bin/bash
# Check if EditOperation.execute validates parent existence
rg -n "execute" --type ts packages/sdk/src/document/operation/edit_operation.ts -B 2 -A 15 | head -100Repository: yorkie-team/yorkie-js-sdk
Length of output: 2058
Reconciliation correctly scopes to specific text instances, but normalizePos lacks defensive error handling.
The reconciliation logic properly uses the operation's parentCreatedAt to scope reconciliation to the correct text instance. However, if normalizePos throws (e.g., if the parent object was removed), the exception would propagate and potentially break the change application flow. While EditOperation.execute() already validates the parent exists before this code path runs, adding defensive error handling around normalizePos would improve robustness against edge cases or concurrent removals.
🤖 Prompt for AI Agents
In packages/sdk/src/document/document.ts around lines 1453 to 1469, normalizePos
may throw (e.g., parent removed) and currently would propagate and break change
application; wrap the call to op.normalizePos(this.root) in a try/catch, and on
error skip reconciliation for that operation (optionally log/debug the failure)
so the change application continues; only call internalHistory.reconcileTextEdit
when normalizePos returns successfully.
What this PR does / why we need it?
Implement reverse of
EditOperationbased on ideas of #1090.This PR only covers undo/redo of Text's edit operation, style is not implemented yet.
It consists of the following three major components:
Any background context you want to provide?
Please read #1090 to understand basis of our undo/redo architecture.
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.