Add intent-preserving Yorkie edits for Docs editor (Phase 1-3)#103
Merged
Conversation
Add character-level Yorkie Tree editing methods that use resolveOffset and resolveDeleteRange helpers instead of full block replacement, enabling efficient collaborative text edits. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Yorkie Tree paths for text editing within inline elements use 3 levels [blockIdx, inlineIdx, charOffset], not 4 levels. The inline node's hasTextChild() causes the last path element to be interpreted as a character offset within its text children automatically.
Use in-place cache update (same pattern as updateBlock) instead of cache invalidation. The invalidation approach caused intermediate states visible to remote peers, triggering "Block not found" errors in the formatting toolbar during concurrent editing.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Implement applyStyle in YorkieDocStore using styleByPath per inline segment (Task 8) and route Doc.applyInlineStyle single-block top-level case through store.applyStyle instead of the local applyStyleToBlock helper (Task 9). Also move superscript/subscript mutual exclusion into the shared applyInlineStyle block-helper so all code paths enforce it. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Implements applySplitBlock and applyMergeBlocks pure helpers in block-helpers.ts, adds splitBlock/mergeBlock to the DocStore interface, and wires them up in MemDocStore with full test coverage. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Implements splitBlock/mergeBlock in YorkieDocStore using editByPath for atomic block replacement/insertion. Wires Doc.splitBlock and Doc.mergeBlocks top-level paths to these new store methods, removing the manual inline manipulation. Also fixes applySplitBlock to preserve listKind/listLevel when the new block type is list-item. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Yorkie's styleByPath sets attributes on a single element node, not on a text range. It cannot split inline nodes for partial styling. Use block-level editByPath replacement until Yorkie adds text-range style support.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds design/task docs and implements intent-preserving, character-level Docs edits: new pure block-helpers, expanded DocStore API (insert/delete/style/split/merge), MemDocStore and YorkieDocStore implementations, Doc delegation to store, tests, and re-exports for helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor/UI
participant Doc as Doc
participant Store as DocStore
participant Helpers as block-helpers
participant Yorkie as Yorkie Tree
participant Cache as Local Cache
Editor->>Doc: insertText(pos, text)
rect rgba(100, 150, 200, 0.5)
Note over Doc: check blockParentMap (table-cell?)
end
alt Non-table-cell
Doc->>Store: insertText(blockId, offset, text)
Store->>Helpers: applyInsertText(block, offset, text)
Helpers-->>Store: updated Block
alt Yorkie-backed
Store->>Yorkie: tree.editByPath(...) (char/edit or block replace)
Yorkie-->>Store: change event
Store->>Cache: update cached doc.blocks (in-place)
else Memory-backed
Store->>Store: replace block in this.doc.blocks
end
else Table-cell
Doc->>Doc: existing in-method cell edit flow
end
Store-->>Doc: void
Doc-->>Editor: void
sequenceDiagram
participant Editor as Editor/UI
participant Doc as Doc
participant Store as DocStore
participant Helpers as block-helpers
participant Yorkie as Yorkie Tree
participant Cache as Local Cache
Editor->>Doc: deleteText(pos, length)
rect rgba(150, 100, 200, 0.5)
Note over Doc: check blockParentMap (table-cell?)
end
alt Non-table-cell
Doc->>Store: deleteText(blockId, offset, length)
Store->>Helpers: resolveDeleteRange(block, offset, length)
Helpers-->>Store: InlineSegment[]
Note over Store: perform deletions (reverse inline order) and normalize
Store->>Helpers: applyDeleteText(...)
Helpers-->>Store: updated Block
alt Yorkie-backed
Store->>Yorkie: tree.editByPath(...) (char deletes / block replace)
Yorkie-->>Store: change event
Store->>Cache: update cached doc.blocks (in-place)
else Memory-backed
Store->>Store: replace block in this.doc.blocks
end
else Table-cell
Doc->>Doc: existing in-method cell delete flow
end
Store-->>Doc: void
Doc-->>Editor: void
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 113.3s
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: 6
🤖 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/design/docs-intent-preserving-edits.md`:
- Around line 102-109: Update the doc to separate shipped Phase 1-3 behavior
from planned Yorkie work: change the table and nearby prose so editByPath is
shown as the character-level API used for text insert/delete, block replacement
is listed for style/structure changes (not styleByPath/splitByPath/mergeByPath),
and editBulkByPath remains a full-document write/undo-fallback; mark
styleByPath, splitByPath, mergeByPath and any *InCell variants as “future work”
(or update examples to use current block-replacement patterns), and add a short
note in the linked README that those APIs are not part of this PR; apply the
same edits to the other mentioned sections referencing
styleByPath/splitByPath/mergeByPath/editByPath/editBulkByPath so examples match
actual shipped behavior.
In `@docs/tasks/active/20260402-intent-preserving-edits-todo.md`:
- Line 3: Update the active task sheet to reflect the shipped phases by removing
or marking completed Phase 1-3 steps and updating the agent guidance that
currently prescribes the discarded four-segment Yorkie paths and functions
styleByPath / splitByPath / mergeByPath; specifically, edit the checklist items
so Phase 1-3 are checked off or moved to a "completed" section, and revise the
agentic-workers instruction block (the sentence referencing
superpowers:subagent-driven-development or superpowers:executing-plans) to only
include the remaining Phase 4-5 actionable steps and the correct current
APIs/approach.
In `@packages/docs/src/model/document.ts`:
- Around line 143-165: The deletion loop in the current code stops when an
inline's available chars equals 0 because offset isn't advanced across inline
boundaries; replace the manual while-loop with the shared block-level deletion
helper from packages/docs/src/store/block-helpers.ts (the same utility used
elsewhere) so deletions traverse inline boundaries correctly. Locate the code
around resolveOffset, normalizeInlines, and updateBlockInStore and call the
block-helpers' delete/remove-text-in-block helper (the function exported in
block-helpers.ts that handles multi-inline deletions) passing the block,
pos.offset and count, then call normalizeInlines(block) and
updateBlockInStore(pos.blockId, block).
In `@packages/docs/src/store/block-helpers.ts`:
- Around line 147-173: The split logic loses the block's cursorStyle when
normalizeInlines turns an empty inline array into a plain {} — preserve
cursorStyle from the original block (or from the side that still has style) when
assigning before.inlines and after.inlines; update the assignment after
normalizeInlines to detect an empty/{} result and copy block.cursorStyle (or
existing cursorStyle on before/after) into the resulting object so splits at run
boundaries keep the formatting; refer to normalizeInlines, before.inlines,
after.inlines, block.inlines and cursorStyle to locate and implement this
change.
In `@packages/docs/src/store/memory.ts`:
- Around line 167-173: In mergeBlock(blockId: string, nextBlockId: string)
ensure you reject merging a block with itself and reject non-adjacent merges:
check that blockId !== nextBlockId and that nextIndex === index + 1 (where index
and nextIndex are located via this.doc.blocks.findIndex), and throw a clear
Error when those guards fail; mirror the same guard logic in
YorkieDocStore.mergeBlock() / DocStore docs and add negative regression tests
for both "same-ID" and "non-adjacent nextBlockId" cases to prevent regressions.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 487-505: deleteText currently issues only character-range edits
via tree.editByPath which leaves empty inline nodes in the Yorkie tree while
applyDeleteText() normalizes the cached block (removing empty inlines and
merging neighbors), causing divergence when treeToDocument() re-parses; fix by
replacing the whole block in deleteText using the same block-replacement pattern
as applyStyle: call tree.editByPath([blockIdx], [blockIdx + 1],
buildBlockNode(applyDeleteText(block, offset, length))) so the Yorkie tree
matches the normalized cache, or if you prefer minimal edits, after the
character-range deletes run targeted cleanup using mergeByPath()/editByPath() to
remove or merge empty inline nodes so the tree and cachedDoc remain consistent.
🪄 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: dd9db341-32f5-437a-ac95-4f8a810e5b96
📒 Files selected for processing (11)
docs/design/README.mddocs/design/docs-intent-preserving-edits.mddocs/tasks/active/20260402-intent-preserving-edits-todo.mdpackages/docs/src/index.tspackages/docs/src/model/document.tspackages/docs/src/store/block-helpers.tspackages/docs/src/store/memory.tspackages/docs/src/store/store.tspackages/docs/test/store/block-helpers.test.tspackages/docs/test/store/memory.test.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/tasks/active/20260402-intent-preserving-edits-todo.md (1)
887-932:⚠️ Potential issue | 🟡 MinorCorrect Task 8 implementation to reflect block-level replacement approach.
This task is marked complete but the prescribed implementation (lines 907-912) uses
tree.styleByPath(), which contradicts the PR summary stating that "style operations remain block-level LWW because Yorkie does not support text-range styling." Update the implementation code snippet to show the block replacement approach that was actually shipped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tasks/active/20260402-intent-preserving-edits-todo.md` around lines 887 - 932, The current applyStyle implementation incorrectly calls tree.styleByPath for each segment; instead implement the block-level LWW replace approach: use getBlock(blockId) and resolveStyleRange(...) to get segments, build a new block value by applying the serialized style (serializeInlineStyle(style)) to the affected inline segments (merging/replacing inline segment style slices per the segments), then inside this.doc.update replace the whole block at blockIdx (found via findBlockIndex(blockId)) with the newly constructed block value (e.g., set/replace the root.content entry at blockIdx) so styling is applied by replacing the block rather than calling tree.styleByPath per-range.
🤖 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/20260402-intent-preserving-edits-todo.md`:
- Line 1: Create a paired lessons file named
docs/tasks/active/20260402-intent-preserving-edits-lessons.md as required by the
coding guidelines; the new file should document learnings from the completed
Phase 1–3 implementation (referenced by the todo file title "Intent-Preserving
Yorkie Edits — Implementation Plan"), including justification for choosing block
replacement over Yorkie path-based methods for styling and structural edits, any
trade-offs, pitfalls encountered, and recommended next steps or follow-ups so
the todo and lessons files are paired and the rationale is clearly captured.
- Line 9: Update the tech stack line to accurately reflect the shipped
implementation: keep TypeScript, Vitest, and Yorkie Tree CRDT but remove
references to Yorkie methods that were not used (styleByPath, splitByPath,
mergeByPath) and state that only editByPath is used for character-level edits
while style and structural operations are performed via block-level replacement
due to Yorkie limitations; modify the tech stack description text accordingly to
mention block-level replacement for style/structural ops and the use of
editByPath for character edits.
---
Duplicate comments:
In `@docs/tasks/active/20260402-intent-preserving-edits-todo.md`:
- Around line 887-932: The current applyStyle implementation incorrectly calls
tree.styleByPath for each segment; instead implement the block-level LWW replace
approach: use getBlock(blockId) and resolveStyleRange(...) to get segments,
build a new block value by applying the serialized style
(serializeInlineStyle(style)) to the affected inline segments (merging/replacing
inline segment style slices per the segments), then inside this.doc.update
replace the whole block at blockIdx (found via findBlockIndex(blockId)) with the
newly constructed block value (e.g., set/replace the root.content entry at
blockIdx) so styling is applied by replacing the block rather than calling
tree.styleByPath per-range.
🪄 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: 0094bccd-bb09-4a20-a0cc-4fdefb72fdb5
📒 Files selected for processing (1)
docs/tasks/active/20260402-intent-preserving-edits-todo.md
Address CodeRabbit review feedback: - Use shared applyDeleteText helper in Doc.deleteText cell fallback path to fix cross-inline boundary deletion bug - Switch YorkieDocStore.deleteText to block replacement to prevent empty inline nodes from diverging between tree and cache - Add self-merge guard to mergeBlock in MemDocStore and YorkieDocStore
- Reflect that only editByPath is used; styleByPath/splitByPath/ mergeByPath are documented as future work - Fix path format references (3 levels, not 4) - Add lessons-learned file for Phase 1-3
Instead of block replacement, use character-level editByPath for text deletion and then remove any resulting empty inline nodes within the same doc.update(). This preserves CRDT merge semantics for concurrent deletions in the same block.
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/docs/src/model/document.ts (1)
229-248:⚠️ Potential issue | 🟠 MajorGuard self-merges before the table-cell branch.
If
blockId === nextBlockIdfor a cell-internal block,idx === nextIdx, so this path concatenates the block with itself and then splices that same entry out ofcell.blocks. The top-level path now fails safely in the store; the cell path should do the same.Suggested fix
mergeBlocks(blockId: string, nextBlockId: string): void { + if (blockId === nextBlockId) { + throw new Error('Cannot merge a block with itself'); + } + const cellInfo = this._blockParentMap.get(blockId); if (cellInfo) { const tableBlock = this.getBlock(cellInfo.tableBlockId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/document.ts` around lines 229 - 248, In mergeBlocks, guard against self-merges by checking if blockId === nextBlockId and returning early before the table-cell branch; specifically, inside mergeBlocks (which uses cellInfo, idx, nextIdx, cell.blocks, block and nextBlock) add a pre-check that immediately returns when the two IDs are identical so you don't concatenate a block with itself or splice the wrong entry for the table cell case.
♻️ Duplicate comments (2)
docs/design/docs-intent-preserving-edits.md (2)
10-13:⚠️ Potential issue | 🟡 MinorSeparate Phase 4 cell edits from the shipped Phase 1-3 scope.
The summary/impact table still read as if same-cell intent-preserving edits already shipped, and this interface section lists
*InCellmethods as newly added. The currentDocStoresurface in this PR only adds the top-level text/style/split/merge methods, so these parts should be marked as Phase 4 future work instead.Also applies to: 53-58, 175-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 10 - 13, Update the docs to clearly separate shipped Phase 1–3 features from future Phase 4 work: change wording in the summary/impact table and the interface section so that character-level in-cell editing (the `*InCell` methods and same-cell intent-preserving edits) are labeled as Phase 4 future work rather than shipped; ensure references to `DocStore` only describe the currently added top-level methods (text/style/split/merge) and remove or reclassify any statements implying `editByPath([idx],[idx+1], blockNode)` was replaced by Yorkie Tree in the shipped scope; apply the same changes to the other occurrences noted (lines ~53–58 and ~175–200).
121-123:⚠️ Potential issue | 🟡 MinorMake the Yorkie text-path examples internally consistent.
Lines 121-123 and 129-131 describe a 3-segment text path, but this example still shows
[blockIdx, inlineIdx, 0, charOffset]. One of these examples is stale, and readers will not know which path shape the currentYorkieDocStoreactually uses.#!/bin/bash set -euo pipefail yorkie_file="$(fd 'yorkie-doc-store\.ts$' . | head -n1)" echo "== Path examples in docs/design/docs-intent-preserving-edits.md ==" rg -n -C1 '\[blockIdx, inlineIdx|editByPath' docs/design/docs-intent-preserving-edits.md if [[ -n "${yorkie_file}" ]]; then echo echo "== Current YorkieDocStore tree-edit usage ==" rg -n -C2 'editByPath|styleByPath|splitByPath|mergeByPath' "${yorkie_file}" fiAlso applies to: 129-131, 289-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 121 - 123, The docs show inconsistent Yorkie text-path shapes (some examples use 3 segments [blockIdx, inlineIdx, charOffset] while others show a stale 4-segment form [blockIdx, inlineIdx, 0, charOffset]); update the examples to match the actual shape used by YorkieDocStore by choosing the 3-segment convention and replacing any occurrences of the extra 0 segment in the examples at lines referenced (also update the examples near editByPath/styleByPath/splitByPath/mergeByPath to use the same form); ensure the text mentions hasTextChild() behavior consistently and run a quick grep against YorkieDocStore (editByPath, styleByPath, splitByPath, mergeByPath) to confirm the chosen path shape matches implementation.
🧹 Nitpick comments (1)
docs/design/docs-intent-preserving-edits.md (1)
231-236: Add languages to the unlabeled fences.Both fences trigger MD040 right now.
textorplaintextis enough here.Also applies to: 358-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 231 - 236, The markdown code fences that list the package tree (the blocks showing "packages/docs/src/store/ ├── store.ts ..." and the similar block later) are unlabeled and trigger MD040; add a language identifier such as "text" or "plaintext" to both fenced blocks (the one containing the packages/docs/src/store listing and the other at lines ~358-376) so the fences become ```text ... ``` to satisfy the linter.
🤖 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/design/docs-intent-preserving-edits.md`:
- Around line 267-268: The docs list normalizeInlines with the wrong type
signature (Block -> Block); update the documentation to the actual signature
used in code: normalizeInlines(inlines: Inline[]): Inline[] and mention its
purpose (merge adjacent Inline entries with identical styles) so readers see the
correct API shape and intent; locate references to normalizeInlines and replace
the incorrect Block->Block declaration with the Inline[] -> Inline[] form.
---
Outside diff comments:
In `@packages/docs/src/model/document.ts`:
- Around line 229-248: In mergeBlocks, guard against self-merges by checking if
blockId === nextBlockId and returning early before the table-cell branch;
specifically, inside mergeBlocks (which uses cellInfo, idx, nextIdx,
cell.blocks, block and nextBlock) add a pre-check that immediately returns when
the two IDs are identical so you don't concatenate a block with itself or splice
the wrong entry for the table cell case.
---
Duplicate comments:
In `@docs/design/docs-intent-preserving-edits.md`:
- Around line 10-13: Update the docs to clearly separate shipped Phase 1–3
features from future Phase 4 work: change wording in the summary/impact table
and the interface section so that character-level in-cell editing (the `*InCell`
methods and same-cell intent-preserving edits) are labeled as Phase 4 future
work rather than shipped; ensure references to `DocStore` only describe the
currently added top-level methods (text/style/split/merge) and remove or
reclassify any statements implying `editByPath([idx],[idx+1], blockNode)` was
replaced by Yorkie Tree in the shipped scope; apply the same changes to the
other occurrences noted (lines ~53–58 and ~175–200).
- Around line 121-123: The docs show inconsistent Yorkie text-path shapes (some
examples use 3 segments [blockIdx, inlineIdx, charOffset] while others show a
stale 4-segment form [blockIdx, inlineIdx, 0, charOffset]); update the examples
to match the actual shape used by YorkieDocStore by choosing the 3-segment
convention and replacing any occurrences of the extra 0 segment in the examples
at lines referenced (also update the examples near
editByPath/styleByPath/splitByPath/mergeByPath to use the same form); ensure the
text mentions hasTextChild() behavior consistently and run a quick grep against
YorkieDocStore (editByPath, styleByPath, splitByPath, mergeByPath) to confirm
the chosen path shape matches implementation.
---
Nitpick comments:
In `@docs/design/docs-intent-preserving-edits.md`:
- Around line 231-236: The markdown code fences that list the package tree (the
blocks showing "packages/docs/src/store/ ├── store.ts ..." and the similar block
later) are unlabeled and trigger MD040; add a language identifier such as "text"
or "plaintext" to both fenced blocks (the one containing the
packages/docs/src/store listing and the other at lines ~358-376) so the fences
become ```text ... ``` to satisfy the linter.
🪄 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: 8bd5a5dc-4f2b-429b-9bf2-a032ff79b726
📒 Files selected for processing (6)
docs/design/docs-intent-preserving-edits.mddocs/tasks/active/20260402-intent-preserving-edits-lessons.mddocs/tasks/active/20260402-intent-preserving-edits-todo.mdpackages/docs/src/model/document.tspackages/docs/src/store/memory.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
✅ Files skipped from review due to trivial changes (2)
- docs/tasks/active/20260402-intent-preserving-edits-lessons.md
- docs/tasks/active/20260402-intent-preserving-edits-todo.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/docs/src/store/memory.ts
- packages/frontend/src/app/docs/yorkie-doc-store.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/design/docs-intent-preserving-edits.md (1)
268-269:⚠️ Potential issue | 🟡 MinorThe
normalizeInlinessignature still showsBlock → Block.This was flagged in a previous review as incorrect. The actual implementation takes
Inline[]and returnsInline[], notBlock → Block.📝 Proposed fix
-// Merge adjacent inlines with identical styles -function normalizeInlines(block: Block): Block; +// Merge adjacent inlines with identical styles +function normalizeInlines(inlines: Inline[]): Inline[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 268 - 269, The function signature for normalizeInlines is incorrect in the docs: it currently shows "function normalizeInlines(block: Block): Block" but the implementation operates on Inline[] and returns Inline[]; update the declaration in the docs to "function normalizeInlines(inlines: Inline[]): Inline[]" (or equivalent wording) so the documented signature matches the actual function name normalizeInlines and its parameter/return types (Inline[]).
🧹 Nitpick comments (3)
docs/design/docs-intent-preserving-edits.md (2)
360-378: Add language specifier to fenced code block.The migration strategy block lacks a language identifier. Consider adding
textfor consistency.♻️ Proposed fix
-``` +```text Phase 1: Doc.insertText(), Doc.deleteText()As per coding guidelines, well-formed markdown improves documentation quality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 360 - 378, The fenced code block listing the migration phases (the block starting with "Phase 1: Doc.insertText(), Doc.deleteText()" and containing Phase 1–5) is missing a language specifier; update the opening fence from ``` to ```text so the block becomes a well-formed labeled code fence (e.g., for the block that includes Doc.insertText(), store.insertText(), Doc.applyInlineStyle() and the Phase 3/Phase 4/Phase 5 entries).
232-237: Add language specifier to fenced code block.The fenced code block showing the directory structure lacks a language identifier. Consider adding
textor an appropriate identifier for consistent formatting.♻️ Proposed fix
-``` +```text packages/docs/src/store/ ├── store.ts # DocStore interfaceAs per coding guidelines, well-formed markdown improves documentation quality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-intent-preserving-edits.md` around lines 232 - 237, The fenced code block showing the directory listing in docs/design/docs-intent-preserving-edits.md is missing a language specifier; update the opening triple backticks to include a language (e.g., use ```text) so the block renders consistently—apply this change to the block that lists packages/docs/src/store/ with entries like store.ts, memory.ts, and block-helpers.ts.packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
630-630: Make the missing-block error more diagnostic.Line 630 currently throws a generic message; including both IDs will speed up production debugging.
As per coding guidelines, "Use meaningful error messages that help with debugging".
🤖 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` at line 630, The thrown error at the check using blockIdx and nextIdx is too generic; update the throw in the block-check (the line that currently does `if (blockIdx === -1 || nextIdx === -1) throw new Error('Block not found')`) to include the offending identifiers (blockIdx and nextIdx) and any nearby context identifiers available in the scope (e.g., blockId/nextId or a document/operation id) so the error message is diagnostic and shows the values that caused the failure.
🤖 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/design/docs-intent-preserving-edits.md`:
- Around line 286-295: The example uses root.content.editByPath with an
incorrect 4-element path; remove the extra 0 so both call sites pass a 3-element
path [blockIdx, inlineIndex, charOffset]. Update the code around resolveOffset,
findBlockIndex and the this.doc.update / root.content.editByPath example to use
[blockIdx, inlineIndex, charOffset] for both the start and end path arguments to
match the documented format.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 625-650: The mergeBlock method currently allows merging
non-adjacent blocks; update mergeBlock(blockId: string, nextBlockId: string) to
enforce that nextBlockId is exactly the immediate successor of blockId by
checking that nextIdx === blockIdx + 1 (after locating blockIdx and nextIdx via
getDocument()); if not, throw a clear Error('Can only merge adjacent blocks').
Also remove or simplify the unnecessary deleteIdx calculation and ensure
tree.editByPath uses blockIdx and blockIdx+1 for deletion so the DOM and
cachedDoc updates stay consistent with the adjacency check.
---
Duplicate comments:
In `@docs/design/docs-intent-preserving-edits.md`:
- Around line 268-269: The function signature for normalizeInlines is incorrect
in the docs: it currently shows "function normalizeInlines(block: Block): Block"
but the implementation operates on Inline[] and returns Inline[]; update the
declaration in the docs to "function normalizeInlines(inlines: Inline[]):
Inline[]" (or equivalent wording) so the documented signature matches the actual
function name normalizeInlines and its parameter/return types (Inline[]).
---
Nitpick comments:
In `@docs/design/docs-intent-preserving-edits.md`:
- Around line 360-378: The fenced code block listing the migration phases (the
block starting with "Phase 1: Doc.insertText(), Doc.deleteText()" and containing
Phase 1–5) is missing a language specifier; update the opening fence from ``` to
```text so the block becomes a well-formed labeled code fence (e.g., for the
block that includes Doc.insertText(), store.insertText(), Doc.applyInlineStyle()
and the Phase 3/Phase 4/Phase 5 entries).
- Around line 232-237: The fenced code block showing the directory listing in
docs/design/docs-intent-preserving-edits.md is missing a language specifier;
update the opening triple backticks to include a language (e.g., use ```text) so
the block renders consistently—apply this change to the block that lists
packages/docs/src/store/ with entries like store.ts, memory.ts, and
block-helpers.ts.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Line 630: The thrown error at the check using blockIdx and nextIdx is too
generic; update the throw in the block-check (the line that currently does `if
(blockIdx === -1 || nextIdx === -1) throw new Error('Block not found')`) to
include the offending identifiers (blockIdx and nextIdx) and any nearby context
identifiers available in the scope (e.g., blockId/nextId or a document/operation
id) so the error message is diagnostic and shows the values that caused the
failure.
🪄 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: a383dd50-1354-4ed5-9bf8-79ff413bfa79
📒 Files selected for processing (3)
docs/design/docs-intent-preserving-edits.mddocs/tasks/active/20260402-intent-preserving-edits-lessons.mdpackages/frontend/src/app/docs/yorkie-doc-store.ts
✅ Files skipped from review due to trivial changes (1)
- docs/tasks/active/20260402-intent-preserving-edits-lessons.md
Includes fix for tree undo/redo with mixed character-level and block-level operations (yorkie-team/yorkie-js-sdk#1196), which is needed for Phase 5 Yorkie-native undo/redo.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/package.json (1)
50-77:⚠️ Potential issue | 🟠 MajorMove
@yorkie-js/sdkfromdevDependenciestodependencies.Runtime code imports
@yorkie-js/sdkinpackages/frontend/src/app/docs/yorkie-doc-store.ts(line 2) andpackages/frontend/src/types/docs-document.ts(line 1). Currently, the package is only indevDependencies, which will break production installs usingpnpm install --prod.Proposed fix
"dependencies": { "@dnd-kit/core": "^6.3.1", "@dnd-kit/modifiers": "^9.0.0", "@dnd-kit/sortable": "^10.0.0", "@dnd-kit/utilities": "^3.2.2", "@wafflebase/docs": "workspace:*", "@wafflebase/sheets": "workspace:*", "@yorkie-js/react": "0.7.3", + "@yorkie-js/sdk": "0.7.3", "class-variance-authority": "^0.7.1", "devDependencies": { - "@yorkie-js/sdk": "0.7.3", "@types/node": "^22.14.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/package.json` around lines 50 - 77, The dependency `@yorkie-js/sdk` is used at runtime (imported in packages/frontend/src/app/docs/yorkie-doc-store.ts and packages/frontend/src/types/docs-document.ts) but is listed under devDependencies; move "@yorkie-js/sdk": "0.7.3" out of devDependencies into the top-level dependencies object in packages/frontend/package.json so it is installed in production builds (remove the entry from devDependencies and add the same version string under dependencies).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/frontend/package.json`:
- Around line 50-77: The dependency `@yorkie-js/sdk` is used at runtime (imported
in packages/frontend/src/app/docs/yorkie-doc-store.ts and
packages/frontend/src/types/docs-document.ts) but is listed under
devDependencies; move "@yorkie-js/sdk": "0.7.3" out of devDependencies into the
top-level dependencies object in packages/frontend/package.json so it is
installed in production builds (remove the entry from devDependencies and add
the same version string under dependencies).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f44801a2-a710-48e8-b39f-8b36c04b370e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/backend/package.jsonpackages/frontend/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/backend/package.json
- Condense design doc to final architecture decisions only - Split monolithic task file into Phase 1-3 (completed), Phase 4 (table cells), and Phase 5 (undo/redo) - Update tasks index
When splitting at the end of bold/italic text, the empty new block
now inherits the style at the split point instead of falling back
to plain {}.
This was referenced Apr 10, 2026
This was referenced Apr 23, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
block-helpers.tsshared by MemDocStore and YorkieDocStore@yorkie-js/sdkto 0.7.3Yorkie Tree Strategy
editByPatheditByPatheditByPatheditByPathstyleByPathis element-level only (not text-range), so style operations use block replacement.Reported upstream: yorkie-team/yorkie-js-sdk#1197
What Changed
New files:
packages/docs/src/store/block-helpers.ts— Pure helper functions (252 lines)packages/docs/test/store/block-helpers.test.ts— 32 unit testsModified files:
DocStoreinterface — AddedinsertText,deleteText,applyStyle,splitBlock,mergeBlockMemDocStore— Implements new methods using shared helpersYorkieDocStore— Character-leveleditByPathfor text, block replacement for style/structureDocclass — Routes to new store methods for top-level blocks, keeps existing path for table cells@yorkie-js/sdk— 0.7.3-alpha → 0.7.3 (includes mixed char+block undo/redo fix)Design doc:
docs/design/docs-intent-preserving-edits.mdKnown Issues
Remaining Phases (separate PRs)
docs/tasks/active/20260403-intent-preserving-phase4-table-cells-todo.md)docs/tasks/active/20260403-intent-preserving-phase5-undo-redo-todo.md)Test plan
pnpm verify:fastpasses (339 tests)🤖 Generated with Claude Code