Add intent-preserving cell structural edits (Phase 8)#156
Merged
Conversation
Migrate cell-internal block insert/delete from updateTableCell (full cell replacement) to editByPath (block-level CRDT operations) so concurrent edits within the same cell are preserved. - Add insertBlockAfter(siblingBlockId, block) to DocStore interface, MemDocStore, and YorkieDocStore - Migrate splitBlock HR/page-break in cell to insertBlockAfter - Migrate insertTableInCell to insertBlockAfter - Migrate deleteTableInCell to deleteBlock with empty-cell guard - Add unit tests for insertBlockAfter and cell-internal deleteBlock - Add concurrent integration test (cell block insert + text edit) - Mark Phase 8 as shipped in design doc Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPhase 8 moves cell-internal block insert/delete from full-cell replacement to intent-preserving, block-level CRDT edits. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Doc as Document
participant Store as DocStore
participant Yorkie as YorkieEngine
participant Cache as InMemoryCache
Doc->>Store: insertBlockAfter(siblingId, block)
Store->>Yorkie: resolveBlockTreePath(siblingId) / editByPath(insert node)
Yorkie-->>Store: ack / updated tree
Store->>Cache: update cached doc blocks (splice new block)
Cache-->>Doc: return refreshed cursor/state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 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 121.1s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/tasks/active/20260423-cell-structural-edits-todo.md (2)
394-404: Completion checklist is missing task archive/index stepAfter verify + design update + commit, add a final step to archive and reindex tasks so the task lifecycle is fully closed.
Suggested addition
- [ ] **Step 3: Commit** @@ git add docs/design/docs/docs-intent-preserving-edits.md git commit -m 'Mark Phase 8 (cell structural edits) as shipped'
+- [ ] Step 4: Archive and index tasks
+
+Run:pnpm tasks:archive && pnpm tasks:index
+Expected: Active task archived and task index updated</details> Based on learnings: “Before marking a task done: (1) Update `docs/design/` if architecture changed, (2) Run `pnpm verify:fast` and confirm pass, (3) Archive with `pnpm tasks:archive && pnpm tasks:index`.” <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/tasks/active/20260423-cell-structural-edits-todo.mdaround lines 394 -
404, Add a final checklist item after the commit step to archive and reindex
tasks so the task lifecycle is closed: insert a new "Step 4: Archive and index
tasks" item that instructs runningpnpm tasks:archive && pnpm tasks:index
and states the expected result ("Active task archived and task index updated");
ensure the step appears after the commit commands and follows the same
formatting/style as the other checklist entries.</details> --- `93-94`: **Use repo-relative commands instead of machine-specific absolute paths** The run commands are tied to `/Users/hackerwins/...`, which makes the checklist non-portable for other contributors/agents. Prefer commands from repo root (or `cd "$(git rev-parse --show-toplevel)"` first). Also applies to: 140-141, 283-284, 363-364, 396-397 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/tasks/active/20260423-cell-structural-edits-todo.mdaround lines 93 -
94, Replace machine-specific absolute cd commands (e.g., the string starting
withcd /Users/hackerwins/Development/wafflebase/wafflebase && pnpm ...) with
repo-relative commands so the checklist is portable: either run from repo root
usingpnpm frontend test -- --test-name-pattern "insertBlockAfter"or prepend
a generic safe cd likecd "$(git rev-parse --show-toplevel)" && pnpm frontend test -- --test-name-pattern "insertBlockAfter". Apply the same change to the
other occurrences that contain similar absolute paths/commands (the other
instances noted in the comment).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/tasks/active/20260423-cell-structural-edits-todo.md:
- Around line 384-386: The two fenced code blocks that currently start with
(showing the table row like "| 8 | Cell structural edits (editByPath) | In Progress |") are missing a language identifier and trigger MD040; update both fenced blocks to use a language tag such asmd ortext so the blocks becomemd or ```text respectively, ensuring the examples (the block
containing the "| 8 | Cell structural edits (editByPath) | In Progress |" row
and the other similar block later) include the language identifier.- Around line 1-404: The PR is missing the required paired lessons file for the
non-trivial active task described in 20260423-cell-structural-edits-todo.md;
create a new docs/tasks/active/20260423-cell-structural-edits-lessons.md that
mirrors the todo file and includes a short task summary, implementation notes
referencing insertBlockAfter, MemDocStore.insertBlockAfter,
YorkieDocStore.insertBlockAfter and Document methods
(splitBlock/insertTableInCell/deleteTableInCell), test results/commands run
(unit and integration commands from the todo), any gotchas or concurrency
lessons learned, and the final commit messages so reviewers can see outcome and
rationale; add and commit this new lessons file as part of the PR.In
@packages/frontend/src/app/docs/yorkie-doc-store.ts:
- Around line 1346-1349: The code uses the tree-path index from siblingPath
directly when splicing blocksArray, which is wrong for body blocks because
resolveBlockTreePath() returns indexes offset by the header; update the splice
index to the local index within the returned blocksArray by subtracting the
tree/body offset before splicing. Specifically, compute localIdx as the last
element of siblingPath adjusted by the offset used when mapping tree paths to
doc.blocks (e.g., subtract the body/tree offset produced by resolveBlockTreePath
or the region/treeOffset value) and then call blocksArray.splice(localIdx + 1,
0, block); update the code around getBlocksArrayForPath, resolveBlockTreePath,
siblingPath and the localIdx computation to use this adjusted local index.
Nitpick comments:
In@docs/tasks/active/20260423-cell-structural-edits-todo.md:
- Around line 394-404: Add a final checklist item after the commit step to
archive and reindex tasks so the task lifecycle is closed: insert a new "Step
4: Archive and index tasks" item that instructs runningpnpm tasks:archive && pnpm tasks:indexand states the expected result ("Active task archived and task
index updated"); ensure the step appears after the commit commands and follows
the same formatting/style as the other checklist entries.- Around line 93-94: Replace machine-specific absolute cd commands (e.g., the
string starting withcd /Users/hackerwins/Development/wafflebase/wafflebase && pnpm ...) with repo-relative commands so the checklist is portable: either run
from repo root usingpnpm frontend test -- --test-name-pattern "insertBlockAfter"or prepend a generic safe cd likecd "$(git rev-parse --show-toplevel)" && pnpm frontend test -- --test-name-pattern "insertBlockAfter". Apply the same change to the other occurrences that contain
similar absolute paths/commands (the other instances noted in the comment).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `85acd846-bab2-42d0-ab00-e85a86ece77f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 713422a6c4786f8bdc9a208ff4df9cf9a7b4851f and 2732ec82960b302d876164623b91b57bce73cd3c. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `docs/design/docs/docs-intent-preserving-edits.md` * `docs/tasks/active/20260423-cell-structural-edits-todo.md` * `packages/docs/src/model/document.ts` * `packages/docs/src/store/memory.ts` * `packages/docs/src/store/store.ts` * `packages/frontend/src/app/docs/yorkie-doc-store.ts` * `packages/frontend/tests/app/docs/yorkie-doc-store-concurrent.integration.ts` * `packages/frontend/tests/app/docs/yorkie-doc-store.test.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The tree path includes bodyTreeOffset when a header exists, but the cache array doc.blocks is zero-indexed. Use getRegionBlocks().topIndex for top-level blocks to subtract the offset correctly. Also add lessons file and fix fenced code block language tags. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
Apr 23, 2026
Migrate cell-internal block insert/delete from updateTableCell (full cell replacement) to editByPath (block-level CRDT operations) so concurrent edits within the same cell are preserved. - Add insertBlockAfter(siblingBlockId, block) to DocStore interface, MemDocStore, and YorkieDocStore - Migrate splitBlock HR/page-break in cell to insertBlockAfter - Migrate insertTableInCell to insertBlockAfter - Migrate deleteTableInCell to deleteBlock with empty-cell guard - Fix cache index offset for documents with headers by using getRegionBlocks().topIndex instead of raw tree path index - Add unit tests for insertBlockAfter, cell-internal deleteBlock, and header-document edge case - Add concurrent integration test (cell block insert + text edit) - Mark Phase 8 as shipped in design doc Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
4 tasks
6 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
insertBlockAfter(siblingBlockId, block)to DocStore interface — works for both top-level and cell-internal blocks via unified blockId resolutionsplitBlockHR/page-break in cell,insertTableInCell, anddeleteTableInCellfromupdateTableCell(full cell replacement) to intent-preservingeditByPath/deleteBlockmergeCellscovered cell reset andupdateBlockDirectpaste where full replacement is the intentTest plan
insertBlockAfter(top-level, cell-internal, table block)deleteBlockon cell-internal blockpnpm verify:fast— 580 docs tests, 156 frontend tests all pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation