Add granular table store updates and fix cross-block selection bugs#96
Merged
Conversation
Phase C decomposes whole-block updateBlock() into fine-grained Yorkie Tree editByPath operations for concurrent cell-level editing. Extends DocStore interface with six table-specific methods (insertTableRow, deleteTableRow, insertTableColumn, deleteTableColumn, updateTableCell, updateTableAttrs) so Doc class can express operation intent directly. Also marks Phase B as complete. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
7 tasks covering: helper extraction, interface extension, MemDocStore, YorkieDocStore, Doc class migration, tests, and final verification. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Prepare for Phase C granular table updates by extracting row and cell node builders into standalone functions.
Phase C: insertTableRow, deleteTableRow, insertTableColumn, deleteTableColumn, updateTableCell, updateTableAttrs.
Direct in-memory mutations with deep cloning for consistency with existing MemDocStore patterns.
Each method uses targeted editByPath with nested paths instead of whole-block replacement. Column operations iterate rows within a single doc.update() for atomicity.
Each table operation now calls the appropriate DocStore method (insertTableRow, updateTableCell, etc.) instead of replacing the entire table block. This enables concurrent cell-level editing when backed by YorkieDocStore. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fix mergeBlocks (cell branch) and applyInlineStyle (cross-block same-cell branch) to use updateTableCell instead of updateBlock on the parent table block.
Tests verify each of the 6 methods (insertTableRow, deleteTableRow, insertTableColumn, deleteTableColumn, updateTableCell, updateTableAttrs) preserves other cells and surrounding blocks.
toggleStyle calls getBlockIndex which returns -1 for cell-internal blocks, causing early return without markDirty. Now falls back to markDirty on the parent table block via getCellInfo.
When selecting across paragraphs that span a table block, applyInlineStyle skipped table blocks because it only iterated top-level block.inlines (empty for tables). Now detects table blocks and applies style to every cell block within them.
handleIndent and handleOutdent only operated on the cursor block. Now iterate all blocks in the selection range via a new getBlocksInSelectionRange helper. Same fix applied in both TextEditor (keyboard shortcut) and EditorAPI (toolbar button).
applyBlockStyle and toggleList had a 3-case selection pattern (cell-range, same-cell cross-block, top-level with table traversal) that was duplicated and missing from indent/outdent. Extract forEachBlockInSelection helper in both editor.ts and text-editor.ts to eliminate duplication and fix indent/outdent for cross-table selections.
|
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 4 minutes and 26 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 (2)
📝 WalkthroughWalkthroughThis PR implements Phase C of the table CRDT design by introducing six granular table-specific Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 104.3s
Verification: verify:integrationResult: ✅ PASS |
Remove implementation details (code snippets, implementation order, test strategy) from docs-table-crdt.md, keeping only final design. Mark Phase C as Complete. Archive the Phase C task to docs/tasks/archive.
Update TableCell type to reflect Block[] containers and add references to detailed design docs.
![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 (1)
packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
482-487: Minor inefficiency:getDocument()called twice per granular method.Each granular method calls
findTableIndex()(which callsgetDocument()), then callsgetDocument()again afterdoc.update(). This results in two document clones per operation.Consider caching the document from
findTableIndex()or having it return both the index and the document:♻️ Optional refactor
- private findTableIndex(tableBlockId: string): number { + private findTableIndex(tableBlockId: string): { index: number; currentDoc: Document } { const currentDoc = this.getDocument(); const index = currentDoc.blocks.findIndex((b) => b.id === tableBlockId); if (index === -1) throw new Error(`Table block not found: ${tableBlockId}`); - return index; + return { index, currentDoc }; }Then callers can reuse the returned
currentDocinstead of callinggetDocument()again.🤖 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 482 - 487, findTableIndex currently calls getDocument() internally and callers also call getDocument() again after doc.update, causing duplicate document clones; change findTableIndex to return both the found index and the document snapshot (e.g., return { index, currentDoc }) or provide a cachedDocument along with the index so callers (methods that call findTableIndex and then doc.update) can reuse the returned currentDoc instead of calling getDocument() a second time; update callers of findTableIndex to destructure the returned { index, currentDoc } and use that currentDoc for reads, only calling this.getDocument() again when a fresh post-update snapshot is actually required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/model/document.ts`:
- Around line 363-379: Selection endpoints can mix top-level paragraph blocks
and table cell blocks causing getBlockIndex(cellBlockId) to return -1 and lead
to dereferencing this._document.blocks[-1]; before the top-level block walk
normalize any endpoint that is a cell block to its parent table block id (use
the cell's tableBlockId) or add an explicit mixed-endpoint branch so the later
loop sees valid top-level indices. Update the selection-normalization logic
(where getBlockIndex is called) to detect cell blocks (via block.tableData
presence or cell lookup), replace the cell endpoint id with its parent
tableBlockId, then proceed to call getBlockIndex and run the existing table
handling (applyStyleToBlock, store.updateTableCell) safely.
- Around line 582-591: The persistence pass is re-checking cell.rowSpan/colSpan
after you've already decremented them, causing merged cells that went from 2->1
to be skipped and not written back; fix by capturing the original span values
before any mutation (e.g., store originalRowSpan = cell.rowSpan ?? 1 and
originalColSpan = cell.colSpan ?? 1) and use those originals when evaluating the
predicate in the loops that call this.store.updateTableCell(blockId, r, c,
cell); apply the same change to the analogous column loop referenced (the
646-655 block) so decisions are based on pre-decrement spans rather than the
mutated values.
In `@packages/docs/src/view/editor.ts`:
- Around line 293-296: In the cursor-only fallback, we're dirtying the leaf cell
block (doc.getBlock(cursor.position.blockId)) instead of the owning table, so
replace the markDirty(block.id) call with a markDirty of the owning table block
id (e.g., use block.tableBlockId or traverse parents from doc.getBlock to find
the enclosing table block id) so toolbar/API calls (applyBlockStyle, toggleList,
indent, outdent) trigger table re-layout; still call fn(block) with the leaf
block before marking the table dirty.
---
Nitpick comments:
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 482-487: findTableIndex currently calls getDocument() internally
and callers also call getDocument() again after doc.update, causing duplicate
document clones; change findTableIndex to return both the found index and the
document snapshot (e.g., return { index, currentDoc }) or provide a
cachedDocument along with the index so callers (methods that call findTableIndex
and then doc.update) can reuse the returned currentDoc instead of calling
getDocument() a second time; update callers of findTableIndex to destructure the
returned { index, currentDoc } and use that currentDoc for reads, only calling
this.getDocument() again when a fresh post-update snapshot is actually required.
🪄 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: 0de26c0e-4821-426a-b2c1-da9856c1c0b2
📒 Files selected for processing (11)
docs/design/docs-table-crdt.mddocs/tasks/active/20260329-table-granular-updates-todo.mdpackages/docs/src/model/document.tspackages/docs/src/store/memory.tspackages/docs/src/store/store.tspackages/docs/src/view/editor.tspackages/docs/src/view/text-editor.tspackages/docs/test/model/document.test.tspackages/docs/test/model/table.test.tspackages/frontend/src/app/docs/yorkie-doc-store.tspackages/frontend/tests/app/docs/yorkie-doc-store.test.ts
Comment on lines
+582
to
+591
| // Update cells with adjusted rowSpan | ||
| for (let r = 0; r < rowIndex; r++) { | ||
| for (let c = 0; c < td.rows[r].cells.length; c++) { | ||
| const cell = td.rows[r].cells[c]; | ||
| const rs = cell.rowSpan ?? 1; | ||
| if (r + rs > rowIndex) { | ||
| this.store.updateTableCell(blockId, r, c, cell); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't re-check decremented spans in the persistence pass.
Both follow-up loops inspect rowSpan/colSpan after you've already decremented it. A merge that ends exactly on the deleted edge goes 2 -> 1, no longer satisfies the predicate, and never gets written back to the store. After refresh(), the row/column is gone but the neighboring merged cell still has stale span metadata.
Also applies to: 646-655
🤖 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 582 - 591, The persistence
pass is re-checking cell.rowSpan/colSpan after you've already decremented them,
causing merged cells that went from 2->1 to be skipped and not written back; fix
by capturing the original span values before any mutation (e.g., store
originalRowSpan = cell.rowSpan ?? 1 and originalColSpan = cell.colSpan ?? 1) and
use those originals when evaluating the predicate in the loops that call
this.store.updateTableCell(blockId, r, c, cell); apply the same change to the
analogous column loop referenced (the 646-655 block) so decisions are based on
pre-decrement spans rather than the mutated values.
hackerwins
changed the title
Address CodeRabbit review findings: - Merge rowSpan/colSpan adjust and store persist into one loop so decremented spans (2->1) are written back correctly - Normalize cell endpoints to parent tableBlockId in cross-block applyInlineStyle for mixed paragraph-cell selections - Use tableBlockId for markDirty in cursor-only fallback of forEachBlockInSelection
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/archive/2026/03/20260329-table-granular-updates-todo.md`:
- Around line 733-757: Update "Task 7: Final verification and cleanup" to add an
explicit final step that runs the archive/index commands before closing the
task: append a new checklist item under Step 4 (or as Step 5) that instructs to
run `pnpm tasks:archive && pnpm tasks:index` and confirm completion; also ensure
the checklist bullet explicitly reminds to update docs/design/ when architecture
changed and to run `pnpm verify:fast` prior to archiving so the final flow in
"Task 7: Final verification and cleanup" is fully reproducible and includes the
archive/index step.
- Around line 126-129: Remove the conflicting typecheck line and keep only the
canonical command: delete the initial "pnpm sheets typecheck 2>&1 | head -5"
entry and leave the canonical "cd packages/docs && npx tsc --noEmit 2>&1 | head
-20" command in the Task 2 section so there is a single authoritative
instruction; update surrounding text if needed to reference only the retained
command.
- Around line 344-351: The current updateTableAttrs implementation replaces the
whole table node via doc.update(... editByPath([tIdx], [tIdx + 1],
buildBlockNode(block))) which violates the Phase C granular-update goal;
instead, change updateTableAttrs (used with findTableIndex and getDocument) to
perform a scoped attrs-only mutation by updating only the tableData.columnWidths
path (e.g., call doc.update with a targeted edit that mutates
root.content[tIdx].tableData.columnWidths) rather than rebuilding the entire
block with buildBlockNode, so concurrent cell edits are not overwritten.
🪄 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: 3882203b-7db6-4081-892b-4a2b9491148e
📒 Files selected for processing (5)
docs/design/docs-table-crdt.mddocs/design/docs-wordprocessor-roadmap.mddocs/tasks/README.mddocs/tasks/archive/2026/03/20260329-table-granular-updates-todo.mddocs/tasks/archive/README.md
✅ Files skipped from review due to trivial changes (2)
- docs/tasks/archive/README.md
- docs/tasks/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design/docs-table-crdt.md
Comment on lines
+126
to
+129
| Run: `pnpm sheets typecheck 2>&1 | head -5` (docs package uses the same typecheck) | ||
|
|
||
| Actually, run: `cd packages/docs && npx tsc --noEmit 2>&1 | head -20` | ||
|
|
There was a problem hiding this comment.
Remove conflicting typecheck command in Task 2
This section lists a command and then immediately overrides it with “Actually, run...”. Keep only the canonical command to avoid accidental execution drift in scripted workflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/archive/2026/03/20260329-table-granular-updates-todo.md` around
lines 126 - 129, Remove the conflicting typecheck line and keep only the
canonical command: delete the initial "pnpm sheets typecheck 2>&1 | head -5"
entry and leave the canonical "cd packages/docs && npx tsc --noEmit 2>&1 | head
-20" command in the Task 2 section so there is a single authoritative
instruction; update surrounding text if needed to reference only the retained
command.
Comment on lines
+344
to
+351
| updateTableAttrs(tableBlockId: string, attrs: { cols: number[] }): void { | ||
| const tIdx = this.findTableIndex(tableBlockId); | ||
| const currentDoc = this.getDocument(); | ||
| const block = currentDoc.blocks[tIdx]; | ||
| block.tableData!.columnWidths = attrs.cols; | ||
| this.doc.update((root) => { | ||
| root.content.editByPath([tIdx], [tIdx + 1], buildBlockNode(block)); | ||
| }); |
There was a problem hiding this comment.
Avoid whole-table replacement in updateTableAttrs instructions
This step reintroduces broad block replacement (editByPath([tIdx], [tIdx + 1], buildBlockNode(block))), which conflicts with the Phase C granular-update goal and risks overwriting concurrent cell edits. Keep updateTableAttrs scoped to attrs-only mutation (or a truly targeted path) instead of replacing the full table node.
Suggested doc correction
- updateTableAttrs(tableBlockId: string, attrs: { cols: number[] }): void {
- const tIdx = this.findTableIndex(tableBlockId);
- const currentDoc = this.getDocument();
- const block = currentDoc.blocks[tIdx];
- block.tableData!.columnWidths = attrs.cols;
- this.doc.update((root) => {
- root.content.editByPath([tIdx], [tIdx + 1], buildBlockNode(block));
- });
- this.cachedDoc = currentDoc;
- this.dirty = false;
- }
+ updateTableAttrs(tableBlockId: string, attrs: { cols: number[] }): void {
+ const tIdx = this.findTableIndex(tableBlockId);
+ const currentDoc = this.getDocument();
+ currentDoc.blocks[tIdx].tableData!.columnWidths = [...attrs.cols];
+ this.cachedDoc = currentDoc;
+ this.dirty = false;
+ // Keep this attrs-scoped; do not replace the entire table node.
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/archive/2026/03/20260329-table-granular-updates-todo.md` around
lines 344 - 351, The current updateTableAttrs implementation replaces the whole
table node via doc.update(... editByPath([tIdx], [tIdx + 1],
buildBlockNode(block))) which violates the Phase C granular-update goal;
instead, change updateTableAttrs (used with findTableIndex and getDocument) to
perform a scoped attrs-only mutation by updating only the tableData.columnWidths
path (e.g., call doc.update with a targeted edit that mutates
root.content[tIdx].tableData.columnWidths) rather than rebuilding the entire
block with buildBlockNode, so concurrent cell edits are not overwritten.
Comment on lines
+733
to
+757
| ### Task 7: Final verification and cleanup | ||
|
|
||
| - [ ] **Step 1: Run full verify** | ||
|
|
||
| Run: `pnpm verify:fast` | ||
| Expected: All tests pass. | ||
|
|
||
| - [ ] **Step 2: Verify existing Doc table tests** | ||
|
|
||
| Run: `cd packages/docs && npx vitest run test/model/table.test.ts --reporter verbose` | ||
| Expected: All tests pass — confirms `MemDocStore` granular methods work correctly through `Doc`. | ||
|
|
||
| - [ ] **Step 3: Verify no remaining whole-block updateBlock calls for tables** | ||
|
|
||
| Search for any remaining `store.updateBlock` calls in table methods of `document.ts`: | ||
|
|
||
| ```bash | ||
| grep -n 'store.updateBlock' packages/docs/src/model/document.ts | ||
| ``` | ||
|
|
||
| Expected: Only non-table usages remain (e.g., `updateBlockInStore`'s else branch for top-level blocks, `insertText`, `deleteText`, etc.). No calls within `insertRow`, `deleteRow`, `insertColumn`, `deleteColumn`, `mergeCells`, `splitCell`, `applyCellStyle`, `setColumnWidth`. | ||
|
|
||
| - [ ] **Step 4: Update design doc Phase C status** | ||
|
|
||
| If all verifications pass, no further changes needed — the design doc was already updated at the start of this session. |
There was a problem hiding this comment.
Final checklist should explicitly include archive/index command
Please add an explicit final step to run pnpm tasks:archive && pnpm tasks:index before closure, so the completion flow is fully reproducible in the task doc itself.
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.
🧰 Tools
🪛 LanguageTool
[style] ~757-~757: Consider an alternative to strengthen your wording.
Context: ...status** If all verifications pass, no further changes needed — the design doc was already upd...
(CHANGES_ADJUSTMENTS)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/archive/2026/03/20260329-table-granular-updates-todo.md` around
lines 733 - 757, Update "Task 7: Final verification and cleanup" to add an
explicit final step that runs the archive/index commands before closing the
task: append a new checklist item under Step 4 (or as Step 5) that instructs to
run `pnpm tasks:archive && pnpm tasks:index` and confirm completion; also ensure
the checklist bullet explicitly reminds to update docs/design/ when architecture
changed and to run `pnpm verify:fast` prior to archiving so the final flow in
"Task 7: Final verification and cleanup" is fully reproducible and includes the
archive/index step.
This was referenced Mar 31, 2026
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
updateBlock()for table blocks into fine-grained Yorkie TreeeditByPathoperations so concurrent cell edits merge instead of last-writer-winsinsertTableRow,deleteTableRow,insertTableColumn,deleteTableColumn,updateTableCell,updateTableAttrs)forEachBlockInSelectionhelper to eliminate ~120 lines of duplicated block-level selection iteration ineditor.tsandtext-editor.tsTest plan
pnpm verify:fastpassespnpm verify:selfpasses (builds + tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation