Replace LWW inline styling with native CRDT operations#151
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Use splitLevel=1 to split inline nodes at style boundaries and styleByPath to apply attributes. This preserves concurrent text edits that were previously lost to last-writer-wins block replacement. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Phase 2 now uses splitLevel=1 + styleByPath instead of LWW block replacement. Remove the styleByPath limitation note since SDK 0.7.6 supports the split+style pattern for inline styling. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Cover: text insert+style, non-overlapping styles, overlapping styles, text delete+style, and style+block split convergence. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Superseded by YorkieDocStore-level concurrent tests in yorkie-doc-store-concurrent.integration.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request implements native CRDT inline styling in YorkieDocStore by replacing LWW-based block replacement with a split+style strategy using Yorkie SDK 0.7.6's Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Store as YorkieDocStore
participant Tree as Yorkie Tree
Client->>Store: applyStyle(blockId, fromOffset, toOffset, attrs)
Store->>Tree: Resolve blockId to block path
Store->>Tree: Query inline nodes in range
Store->>Tree: editByPath(toOffset, splitLevel=1)
Note over Tree: Split at toOffset
Tree-->>Store: Path updated
Store->>Tree: editByPath(fromOffset, splitLevel=1)
Note over Tree: Split at fromOffset
Tree-->>Store: Range isolated
Store->>Tree: styleByPath(inline1, attrs)
Store->>Tree: styleByPath(inline2, attrs)
Note over Tree: Apply to contained inlines
Store->>Tree: Remove empty inlines
Tree-->>Store: Cleanup complete
Store-->>Client: Style applied
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Verification: verify:selfResult: ✅ PASS in 115.0s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
819-927:⚠️ Potential issue | 🟡 MinorGuard zero-width ranges to avoid spurious inline splits + cache/tree divergence.
When
fromOffset === toOffsetand the offset lands strictly inside an inline (e.g.,fromOffset = toOffset = 3on'Hello'), thetoOffsetsplit at Line 843 fires because0 < charOffset < inlineTextLenis true. The zero-width guard at Line 883 then correctly skips the styling loop, and the cleanup at Line 915 only removes fully empty inlines, so the extra split persists in the CRDT tree. Meanwhile the cache is rebuilt viaapplyInlineStyleHelper, which for a zero-width range typically produces the unsplit block — socachedDocand the underlying tree diverge until the next remote change invalidates the cache.An early return at the top of
applyStylewould avoid both the spurious CRDT op and the divergence:🛡️ Proposed early exit
applyStyle( blockId: string, fromOffset: number, toOffset: number, style: Partial<InlineStyle>, ): void { + if (fromOffset >= toOffset) return; const currentDoc = this.getDocument();This also lets you drop the zero-width branch at Lines 883-886.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/docs/yorkie-doc-store.ts` around lines 819 - 927, The code performs splits even for zero-width ranges, causing spurious CRDT edits and cache/tree divergence; add an early return at the start of the applyStyle (or caller that receives fromOffset and toOffset, e.g., applyInlineStyleHelper invocation) that checks if fromOffset === toOffset and returns without mutating the tree or cache. Ensure the guard runs before executing the toOffset split logic (the block that calculates toPos/toInline and calls tree.editByPath) so no editByPath is invoked for zero-width ranges, and remove or keep the existing zero-width branch in the loop as desired.
🧹 Nitpick comments (1)
packages/frontend/tests/app/docs/yorkie-doc-store.test.ts (1)
431-546: Consider one tree-level readback test + a zero-width case.All 8 cases assert via
getBlock/getDocument, which returnscachedDocpopulated fromapplyInlineStyleHelper(not by re-parsing the Yorkie tree). That means cache/tree divergence (e.g., the zero-width split side-effect noted onyorkie-doc-store.ts) can't be detected here.Recommended additions (mirrors the pattern already used by
should preserve bold attr in Yorkie Tree after split (not just cache)at lines 600-618):it('should persist bold attr on the Yorkie Tree after applyStyle (not just cache)', () => { const block = makeBlock('HelloWorld'); store.setDocument({ blocks: [block] }); store.applyStyle(block.id, 3, 8, { bold: true }); const tree = doc.getRoot().content; const blockNode = tree.getRootTreeNode().children[0]; const boldInlines = blockNode.children.filter( (c: { attributes?: Record<string, string> }) => c.attributes?.bold === 'true', ); assert.equal(boldInlines.length, 1); }); it('should be a no-op for a zero-width range', () => { const block = makeBlock('Hello'); store.setDocument({ blocks: [block] }); store.applyStyle(block.id, 3, 3, { bold: true }); // Invalidate cache and re-parse from tree to catch cache/tree divergence // `@ts-expect-error` accessing private field for test store.dirty = true; // `@ts-expect-error` accessing private field for test store.cachedDoc = null; const result = store.getBlock(block.id)!; assert.equal(result.inlines.length, 1); assert.equal(result.inlines[0].text, 'Hello'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/app/docs/yorkie-doc-store.test.ts` around lines 431 - 546, Add two tests to detect cache/tree divergence after applyStyle: (1) a Yorkie-tree-level assertion that after store.applyStyle(block.id, 3, 8, { bold: true }) the underlying doc tree contains exactly one inline with attributes.bold === 'true' (use doc.getRoot().content.getRootTreeNode().children to locate block node and inspect children attributes), and (2) a zero-width no-op test that calls store.applyStyle(block.id, 3, 3, { bold: true }), then force reparse by setting store.dirty = true and store.cachedDoc = null before calling store.getBlock(block.id) and asserting the block remains a single inline with text 'Hello'; these mirror the existing pattern used in the other tree-level test and will catch the split/caching bug in applyInlineStyleHelper/applyStyle in yorkie-doc-store.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tasks/active/20260423-native-inline-style-todo.md`:
- Around line 222-260: The snippet contains a dead first loop that computes
startIdx/endIdx over inlines and is immediately discarded when both are reset to
-1; remove that first loop (the block that computes pos, startIdx, endIdx up to
the "Fallback" lines) or replace it with the single-loop shipped algorithm that
walks inlines once to derive startIdx and endIdx based on fromOffset and
toOffset; ensure the implementation consistently uses the same logic over
inlines, fromOffset, toOffset and only computes startIdx/endIdx once (refer to
the startIdx/endIdx/inlines variables in the diff).
---
Outside diff comments:
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 819-927: The code performs splits even for zero-width ranges,
causing spurious CRDT edits and cache/tree divergence; add an early return at
the start of the applyStyle (or caller that receives fromOffset and toOffset,
e.g., applyInlineStyleHelper invocation) that checks if fromOffset === toOffset
and returns without mutating the tree or cache. Ensure the guard runs before
executing the toOffset split logic (the block that calculates toPos/toInline and
calls tree.editByPath) so no editByPath is invoked for zero-width ranges, and
remove or keep the existing zero-width branch in the loop as desired.
---
Nitpick comments:
In `@packages/frontend/tests/app/docs/yorkie-doc-store.test.ts`:
- Around line 431-546: Add two tests to detect cache/tree divergence after
applyStyle: (1) a Yorkie-tree-level assertion that after
store.applyStyle(block.id, 3, 8, { bold: true }) the underlying doc tree
contains exactly one inline with attributes.bold === 'true' (use
doc.getRoot().content.getRootTreeNode().children to locate block node and
inspect children attributes), and (2) a zero-width no-op test that calls
store.applyStyle(block.id, 3, 3, { bold: true }), then force reparse by setting
store.dirty = true and store.cachedDoc = null before calling
store.getBlock(block.id) and asserting the block remains a single inline with
text 'Hello'; these mirror the existing pattern used in the other tree-level
test and will catch the split/caching bug in applyInlineStyleHelper/applyStyle
in yorkie-doc-store.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18013d6d-601f-4934-ba56-77c4beb1061c
📒 Files selected for processing (5)
docs/design/docs/docs-intent-preserving-edits.mddocs/tasks/active/20260423-native-inline-style-todo.mdpackages/frontend/src/app/docs/yorkie-doc-store.tspackages/frontend/tests/app/docs/yorkie-doc-store-concurrent.integration.tspackages/frontend/tests/app/docs/yorkie-doc-store.test.ts
| // Walk inlines to find the range matching [fromOffset, toOffset) | ||
| let pos = 0; | ||
| let startIdx = -1; | ||
| let endIdx = -1; | ||
| for (let i = 0; i < inlines.length; i++) { | ||
| const len = (inlines[i].children ?? []) | ||
| .filter((c): c is { type: 'text'; value: string } => c.type === 'text') | ||
| .reduce((s, t) => s + t.value.length, 0); | ||
| if (startIdx === -1 && pos + len > fromOffset) { | ||
| startIdx = i; | ||
| } | ||
| if (pos + len <= toOffset && pos >= fromOffset) { | ||
| endIdx = i; | ||
| } else if (startIdx !== -1 && pos < toOffset) { | ||
| endIdx = i; | ||
| } | ||
| pos += len; | ||
| } | ||
|
|
||
| // Fallback: if range covers from start | ||
| if (startIdx === -1) startIdx = 0; | ||
| if (endIdx === -1) endIdx = startIdx; | ||
|
|
||
| // Walk inlines: find first that starts at fromOffset, last that ends at toOffset | ||
| pos = 0; | ||
| startIdx = -1; | ||
| endIdx = -1; | ||
| for (let i = 0; i < inlines.length; i++) { | ||
| const len = (inlines[i].children ?? []) | ||
| .filter((c): c is { type: 'text'; value: string } => c.type === 'text') | ||
| .reduce((s, t) => s + t.value.length, 0); | ||
| if (pos >= fromOffset && pos + len <= toOffset) { | ||
| if (startIdx === -1) startIdx = i; | ||
| endIdx = i; | ||
| } | ||
| pos += len; | ||
| } | ||
| if (startIdx === -1) startIdx = 0; | ||
| if (endIdx === -1) endIdx = startIdx; |
There was a problem hiding this comment.
Plan-doc snippet contains a dead first loop that contradicts the shipped implementation.
Lines 222-243 compute startIdx/endIdx and then Lines 246-260 immediately reset both to -1 and recompute with a different algorithm, so the first loop is dead code within the snippet. The actual shipped code in packages/frontend/src/app/docs/yorkie-doc-store.ts (Lines 875-897) uses a single, different algorithm. Either drop the first loop from the snippet or replace the block with the shipped logic to avoid misleading future readers who use this plan as reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/active/20260423-native-inline-style-todo.md` around lines 222 -
260, The snippet contains a dead first loop that computes startIdx/endIdx over
inlines and is immediately discarded when both are reset to -1; remove that
first loop (the block that computes pos, startIdx, endIdx up to the "Fallback"
lines) or replace it with the single-loop shipped algorithm that walks inlines
once to derive startIdx and endIdx based on fromOffset and toOffset; ensure the
implementation consistently uses the same logic over inlines, fromOffset,
toOffset and only computes startIdx/endIdx once (refer to the
startIdx/endIdx/inlines variables in the diff).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
YorkieDocStore.applyStyle()from LWW block replacement to native CRDTsplitLevel=1+styleByPathdocs-intent-preserving-edits.md) Phase 2 statusTest plan
applyStyle8 cases (middle, start, end, entire, multi-inline, after insert, toggle-off, styled insert)pnpm verify:fastpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes