Unify table cell editing pipeline via BlockParentMap#94
Merged
Conversation
Replace TableCell.inlines with TableCell.blocks so each cell holds a mini-document (Block[]) instead of flat Inline[]. This is Phase A of the table CRDT work — the data model and store layer change that enables per-cell concurrent editing via Yorkie Tree nodes. Key changes: - TableCell.inlines → TableCell.blocks (paragraph block per cell) - Doc *InCell methods operate on cell.blocks[0] - YorkieDocStore serializes tables as row→cell→block tree nodes instead of JSON-stringified tableData attribute - Table layout wraps cell blocks instead of cell inlines - All tests updated for the new cell structure Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Mark all Phase A steps as complete and add lessons learned. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Enable Enter key to split a block into two paragraphs within a table cell, and Backspace at a block boundary to merge them back. This makes cell-internal blocks fully functional for basic editing. Key changes: - Add cellBlockIndex to DocPosition for per-block cursor tracking - Add splitBlockInCell, mergeBlocksInCell, getCellBlockTextLength to Doc class - Update all cell editing handlers (Enter, Backspace, Delete, Input, IME composition) to pass cellBlockIndex - Add blockBoundaries to LayoutTableCell for click-to-block mapping - Update resolvePositionPixel to render cursor in correct cell block - Arrow keys navigate across cell block boundaries Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Document-level operations (search, Bold/Italic) were only working on top-level blocks, ignoring table cell content entirely. The cell-aware methods existed but were never connected to the toolbar/keyboard paths. - searchText now descends into table cell blocks, returning matches with cellAddress and cellBlockIndex - toggleStyle and EditorAPI.applyStyle check cellAddress and route to applyCellInlineStyle instead of applyInlineStyle - Find/Replace highlights and replace operations work within cells - SearchMatch type extended with cellAddress and cellBlockIndex fields Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
getStyleAtCursor() and getSelectionStyle() were reading from the top-level table block's empty inlines instead of the cell's block content. This caused all toggle operations (bold, italic, underline, etc.) to always apply but never remove — the current state was always read as undefined/false. - getStyleAtCursor: read from cell block inlines when cellAddress set - getSelectionStyle: same fix for toolbar button state display - clearFormatting: route to applyCellInlineStyle for cells - insertLink/removeLink/getLinkAtCursor: cell-aware routing Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Table cells can now contain list-item, heading, and other block types. Previously, block-type operations (list toggle, indent/outdent, style dropdown) only worked on top-level blocks and ignored cell content. - Add Doc.setBlockTypeInCell() for changing cell block types - Route getBlockType/setBlockType/toggleList/indent/outdent through cellAddress when cursor is in a table cell - Layout: reserve list indent space in layoutCellBlocks - Render: draw bullet/number markers for list-item blocks in cells - Keyboard shortcuts (⌘⇧7/8, ⌘]/[) work inside cells Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
handleArrow had a separate cell-specific branch that bypassed moveLeft/moveRight and constructed positions without cellBlockIndex. This caused arrow left/right to jump between blocks incorrectly — the cursor would land at a flat offset in the first block instead of the correct block. Now delegates to moveLeft/moveRight which properly handle block boundaries and preserve cellBlockIndex. Also adds cellBlockIndex: 0 to up/down cell navigation positions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Selection across multiple blocks within a cell (e.g. selecting from block 0 to block 1) was broken: the selection range disappeared when start and end offsets matched, and styles only applied to one block. Root cause: normalizeRange, hasSelection, and computeSelectionRects compared only offset values without considering cellBlockIndex. - normalizeRange: compare cellBlockIndex before offset - hasSelection: treat different cellBlockIndex as valid selection - computeSelectionRects: different cellBlockIndex is not empty - getSelectedText: concatenate text across cell blocks - Add applyCellStyleToRange helper for cross-block style application Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
resolveOffsetInCell only used X coordinate, iterating all cell lines top-to-bottom and returning the first X match. For cells with identical text in multiple blocks (e.g. "asdf" in both), clicks on the second block always landed in the first. Now uses Y coordinate to first determine which line was clicked, then resolves X within that specific line. The Y is mapped to cell-local coordinates via the paginated layout. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Cell single-click set selection to null, which prevented drag selection from starting — handleMouseMove early-returned when selection.range was null. - Set anchor position on cell click (same as non-cell click) - Use Y-aware resolution for drag selection within cells Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Up/Down in cells skipped cell-internal blocks entirely, jumping straight to the cell above/below or exiting the table. Now moves between blocks within the same cell first, then to adjacent cells. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
moveVertical used paginatedPixelToPosition which returned the table block ID with a character offset — but table blocks have empty inlines, so the cursor landed at an invalid position. Now detects when the result targets a table block and enters the appropriate cell: Down enters first cell (0,0), Up enters last cell. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The canvas container's mousedown handler captured all clicks within the container div, including clicks on context menu buttons rendered inside the same container. This moved the cursor to the click position and prevented the menu action from executing. Now skips mousedown handling when the target is a button, input, or element inside a menu role container. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When drag-selecting within a cell and crossing into another cell, the selection switches from text mode to cell-range mode, highlighting entire cells. When the mouse leaves the table, the selection falls back to block-range mode. Key changes: - Add TableCellRange type and DocRange.tableCellRange field - normalizeRange accepts cell-range selections - updateDragSelection detects cell boundary crossing via resolveTableCellClick and switches to cell-range mode - buildCellRangeRects renders full-cell highlight rectangles - deleteSelection clears cell content for cell-range selections - getSelectedText returns tab/newline-separated cell text Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Hide cursor when tableCellRange is active (no blinking caret) - Add applyStyleToCellRange: applies inline style to all blocks in all cells within the selected range - toggleStyle and applyStyle check tableCellRange before falling back to single-cell style routing Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When dragging from inside a cell to outside the table, select all cells instead of dropping into block-range mode. When dragging from a paragraph across a table block, the table is highlighted as a whole (all cells selected). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
applyBlockStyle and handleAlign only operated on top-level blocks, ignoring cell-internal blocks. Cell layout also didn't apply horizontal alignment to line runs. - Add Doc.applyBlockStyleInCell for cell block styles - Route EditorAPI.applyBlockStyle and handleAlign through cellAddress - Export applyAlignment from layout.ts and use in layoutCellBlocks - Alignment shortcuts (⌘⇧L/E/R/J) work inside cells Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1. splitBlockInCell preserves list-item type and exit behavior 2. IME live update passes cellBlockIndex in handleInput composition 3. mergeCells appends all blocks from covered cells, not just first 4. insertLink uses applyCellStyleToRange for cross-block support 5. buildCellRangeRects uses found flag instead of tablePageY !== 0 6. List marker counter only increments for ordered items, resets on unordered to prevent 1. • 3. numbering Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Right-clicking while a cell-range selection was active triggered handleMouseDown which reset the cursor and selection. Now skips mousedown for right-click (button 2) when tableCellRange is set, so the context menu opens with the selection preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When dragging from inside a cell to outside the table, the selection highlighted all table cells but excluded external paragraphs. The tableCellRange mode locked the focus inside the table. Now switches to block-range selection (no tableCellRange) with the anchor at the table block start and focus at the external position. buildRects already highlights table blocks as full-cell rects within a block-range selection. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Shift+Arrow in cells only supported text selection within the same cell. Crossing cell boundaries or exiting the table was ignored. - Shift+Left/Right at cell boundary: sets tableCellRange for cross-cell selection - Shift+Left at first cell / Shift+Right at last cell: exits table with block-range selection including external content - Shift+Up/Down across cells: sets tableCellRange when cellAddress differs between anchor and focus Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Phase B covers unified editing pipeline: BlockParentMap, cellAddress removal from DocPosition, *InCell method elimination, and editor branch cleanup. Phase C (future) covers granular YorkieDocStore updates for concurrent cell-level CRDT merging. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
12-task plan to remove cellAddress from DocPosition, eliminate *InCell methods, and unify cell/top-level block editing via BlockParentMap reverse lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add BlockCellInfo type that maps cell-internal block IDs to their parent table/cell. The map is built during computeTableLayout and merged into the document-level DocumentLayout. This is the foundation for Phase B of the Table CRDT project, enabling unified editing pipeline without cellAddress in DocPosition. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add BlockParentMap-based lookup so Doc can find and update cell-internal blocks by ID. This is the foundation for removing *InCell methods and cellAddress from DocPosition in later tasks. - Add _blockParentMap field with setter/getter - Extend getBlock() to search cell blocks via BlockParentMap - Add getParentTableBlock() helper - Add updateBlockInStore() to route cell block updates through parent table - Extend splitBlock, mergeBlocks, applyInlineStyle for cell blocks - Update insertText, deleteText, applyBlockStyle, setBlockType to use updateBlockInStore Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ddress Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…n/SearchMatch Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace 193 type errors caused by the removal of cellAddress/cellBlockIndex
from DocPosition and *InCell methods from Doc. All cell operations now use
the unified API (doc.insertText, deleteText, splitBlock, mergeBlocks,
applyInlineStyle, applyBlockStyle, setBlockType) which handles cell blocks
transparently via blockParentMap lookups.
Key changes:
- Add isInCell/getCellInfo helpers using layout.blockParentMap
- IME composition and Hangul assembly use unified doc methods
- Arrow/backspace/delete/enter in cells use cell block IDs directly
- Mouse click/drag selection resolves to cell block IDs
- Remove getCellText, getCellTextLength, applyCellStyleToRange,
resolveOffsetInCellAtX helpers
- moveToNextCell/moveToPrevCell navigate using blockParentMap
- resolveOffsetInCell/AtXY now returns { blockId, offset }
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace removed *InCell method calls (insertTextInCell, deleteTextInCell, applyCellInlineStyle, splitBlockInCell, mergeBlocksInCell, getCellBlockTextLength) with the unified Doc API that routes through BlockParentMap. Fix mergeCells test to check text across all blocks, and fix clipboard test font-size assertion for px-to-pt conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Without this, Doc.getBlock() cannot find cell blocks by ID because the blockParentMap was only on DocumentLayout but never propagated to the Doc instance in production code. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
normalizeRange() was looking up cell block IDs in layout.blocks (which only contains top-level blocks), returning null before reaching the cell-aware logic. Move the blockParentMap check before the layout.blocks index lookup and resolve cell block IDs to their parent table block ID. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
deleteText(pos, -1) was passing a negative length which caused the method to return immediately. Changed to delete 1 character at offset-1 instead. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Shift+Arrow at cell boundary was exiting the table instead of selecting into the adjacent cell. Added getNextCellFirstPosition and getPrevCellLastPosition helpers so Shift+Left/Right moves to the neighboring cell for cross-cell selection, only exiting the table when at the first/last cell. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
applyBlockStyle used getBlockIndex() which returns -1 for cell block IDs, falling through to single-block path. Now handles three cases: cell-range selection (all blocks in selected cells), within-cell multi-block selection, and top-level multi-block. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
applyStyle was calling doc.applyInlineStyle (which succeeded) but then failing to markDirty because getBlockIndex returns -1 for cell block IDs. Now uses blockParentMap to mark the parent table block dirty for cell selections. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The table renderer was missing backgroundColor support for inline styles — fillRect before fillText, matching the same pattern used in doc-canvas.ts for top-level blocks. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add reset button with IconDropletOff, match button styles (cursor-pointer, rounded, hover:scale-125, transition-transform) to the formatting toolbar's text color and highlight palettes. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Resolve conflicts by keeping Phase B (BlockParentMap-based) code over Phase A (cellAddress-based) code from main. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
📝 WalkthroughWalkthroughThis PR implements Phase B of a Table CRDT refactor, removing Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Text Editor
participant Layout as Document Layout
participant Doc as Doc Model
participant Store as Block Store
participant BlockMap as Block Parent Map
Note over Editor,BlockMap: Block Operation with Cell Context (Phase B)
rect rgba(100, 150, 200, 0.5)
Editor->>Layout: computeLayout()
Layout->>Layout: buildBlockParentMap() for table cells
Layout->>BlockMap: Register cell block → tableBlockId, rowIndex, colIndex
Layout-->>Editor: DocumentLayout + blockParentMap
end
rect rgba(200, 150, 100, 0.5)
Editor->>BlockMap: lookup(blockId) for cell context
BlockMap-->>Editor: BlockCellInfo | undefined
Editor->>Doc: splitBlock(blockId, offset)
end
rect rgba(150, 200, 100, 0.5)
Doc->>BlockMap: check if blockId is cell-internal
alt Is cell-internal
Doc->>Doc: fetch parent table block via BlockCellInfo
Doc->>Store: updateBlock(tableBlockId, updatedTableBlock)
else Not cell-internal
Doc->>Store: updateBlock(blockId, updatedBlock)
end
Store-->>Doc: ✓ Block updated
Doc-->>Editor: ✓ Operation complete
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 107.1s
Verification: verify:integrationResult: ✅ PASS |
applyTableCellStyle ignored tableCellRange selection and only applied to the cursor position's cell. Now iterates all cells in the selected range when a multi-cell selection exists. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Remote cell-range selections were not rendered because tableCellRange was stripped when passing selection through onCursorMove → presence → buildPeerCursors. Now propagated through the full pipeline. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![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
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/view/text-editor.ts (1)
1322-1350:⚠️ Potential issue | 🟠 MajorMake Ctrl/Option+Arrow cell-aware across sibling blocks.
Inside a cell, the
wordModbranches still callmoveWordLeft()/moveWordRight(), and those helpers only understand top-level block ordering. AfterEntersplits a cell into multiple blocks, Ctrl/Option+Left/Right gets stuck at the block boundary instead of traversing the adjacent cell block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 1322 - 1350, The Ctrl/Option+Arrow handling uses moveWordLeft()/moveWordRight(), which only consider top-level block order and so stop at intra-cell block boundaries; update the word-mod branch to be cell-aware by either (A) calling cell-aware helpers (e.g., getPrevCellLastPosition/getNextCellFirstPosition) when arrowCellInfo is present, or (B) change moveWordLeft/moveWordRight to accept an optional arrowCellInfo/cell context and have them traverse sibling blocks inside the same cell before moving to adjacent cells; adjust the branch in the direction === 'left'/'right' blocks to use the new cell-aware behavior and fall back to existing logic when no arrowCellInfo is available so Ctrl/Option+Arrow moves across blocks inside the same cell as expected.
🧹 Nitpick comments (2)
packages/docs/src/model/types.ts (1)
355-361: Consider removing unused optional fields fromSearchMatch.The
cellAddressandcellBlockIndexfields remain as optional properties, but per the context snippets,searchText()no longer populates them (it now uses the cell block's ID directly asblockId). These fields appear to be dead code.Removing them would complete the cleanup and prevent confusion:
♻️ Suggested cleanup
export interface SearchMatch { blockId: string; startOffset: number; endOffset: number; - cellAddress?: CellAddress; - cellBlockIndex?: number; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/types.ts` around lines 355 - 361, SearchMatch still declares optional fields cellAddress and cellBlockIndex which are no longer populated by searchText; remove these two properties from the SearchMatch interface and then update any code or tests that reference SearchMatch.cellAddress or SearchMatch.cellBlockIndex (or import CellAddress only for this purpose) so callers use blockId instead; ensure type imports are adjusted and run typechecks to catch remaining references to cellAddress/cellBlockIndex.packages/docs/src/view/peer-cursor.ts (1)
80-84: Defensive handling for cell block index lookup.The fallback to
0whenfindIndexreturns-1is a reasonable defensive measure. However, ifposition.blockIdis inblockParentMapbut not found incellData.blocks, this indicates an inconsistency between the map and the cell data (e.g., stale map after a block was removed).Consider logging a warning when
cbi < 0to aid debugging synchronization issues during development:💡 Optional: Add debug warning for inconsistent state
const cellData = lb.block.tableData?.rows[rowIndex]?.cells[colIndex]; const cbi = cellData ? cellData.blocks.findIndex((b) => b.id === position.blockId) : 0; + if (cbi < 0 && cellData) { + console.warn(`Block ${position.blockId} in blockParentMap but not in cell blocks`); + } const effectiveCbi = cbi >= 0 ? cbi : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/peer-cursor.ts` around lines 80 - 84, The current logic falls back to 0 when findIndex returns -1, masking inconsistencies between blockParentMap and cellData.blocks; update the block index lookup in the peer cursor code (symbols: lb.block.tableData, cellData, cbi, effectiveCbi, position.blockId, blockParentMap) to log a warning when cbi < 0 before using effectiveCbi so developers can detect stale or inconsistent state, keeping the existing fallback to 0 to preserve runtime behavior.
🤖 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/20260329-table-crdt-phase-b-todo.md`:
- Line 1186: Summary: The phrase "outside of" is wordy and should be simplified
to "outside". Edit the sentence that currently reads "Grep for any remaining
`cellAddress` or `cellBlockIndex` references outside of the retained
`CellAddress` type and `TableCellRange`" to instead read "Grep for any remaining
`cellAddress` or `cellBlockIndex` references outside the retained `CellAddress`
type and `TableCellRange`", leaving the identifiers `cellAddress`,
`cellBlockIndex`, `CellAddress`, and `TableCellRange` unchanged.
In `@packages/docs/src/model/document.ts`:
- Around line 771-809: In splitBlock's cell-path code, avoid calling
getStyleAtOffset() when the target block has been converted to a horizontal
rule: add a guard that checks if targetBlock.type === 'horizontal-rule' (same
behavior as the top-level splitBlock guard) and return the current blockId
early; this prevents getStyleAtOffset() from being invoked on an empty inlines
array after setBlockType() clears inlines. Ensure the check is placed before
calling getStyleAtOffset() (around the block that references targetBlock,
buildInlinesFromSplit, and cursorStyle).
In `@packages/docs/src/view/editor.ts`:
- Around line 852-857: The TextEditor helpers that power the cursor-link popover
and Cmd/Ctrl-click currently iterate only doc.document.blocks and therefore miss
links inside cell blocks; update those helpers (the ones calling into
getLinkAtCursor()/removeLink() and any code that directly reads
doc.document.blocks) to resolve the block via the same cell-aware lookup used
elsewhere (use doc.getBlock(cursor.position.blockId) or the blockParentMap
lookup) and then inspect block.inlines for links; ensure both the
popover/Ctrl-click path and any link-removal code use
getLinkAtCursor()/removeLink() or the doc.getBlock-based resolution so links
inside table cells are discovered and handled correctly.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 2182-2194: The Home/End and Cmd+Backspace behavior is wrong
because getVisualLineStart and getVisualLineEnd short-circuit wrapped cell
blocks to whole-block offsets; instead thread a lineAffinity parameter through
getVisualLineRange, getVisualLineStart, and getVisualLineEnd (and callers like
handleLineBackspace) so wrap-boundary offsets resolve to the correct visual
line; remove or replace the isInCell short-circuits in
getVisualLineStart/getVisualLineEnd to call getVisualLineRange(pos,
lineAffinity) and return the start/end from that range, and update callers to
pass the current lineAffinity so Home/End and line-deletion operate on the
visual line rather than the entire block.
---
Outside diff comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 1322-1350: The Ctrl/Option+Arrow handling uses
moveWordLeft()/moveWordRight(), which only consider top-level block order and so
stop at intra-cell block boundaries; update the word-mod branch to be cell-aware
by either (A) calling cell-aware helpers (e.g.,
getPrevCellLastPosition/getNextCellFirstPosition) when arrowCellInfo is present,
or (B) change moveWordLeft/moveWordRight to accept an optional
arrowCellInfo/cell context and have them traverse sibling blocks inside the same
cell before moving to adjacent cells; adjust the branch in the direction ===
'left'/'right' blocks to use the new cell-aware behavior and fall back to
existing logic when no arrowCellInfo is available so Ctrl/Option+Arrow moves
across blocks inside the same cell as expected.
---
Nitpick comments:
In `@packages/docs/src/model/types.ts`:
- Around line 355-361: SearchMatch still declares optional fields cellAddress
and cellBlockIndex which are no longer populated by searchText; remove these two
properties from the SearchMatch interface and then update any code or tests that
reference SearchMatch.cellAddress or SearchMatch.cellBlockIndex (or import
CellAddress only for this purpose) so callers use blockId instead; ensure type
imports are adjusted and run typechecks to catch remaining references to
cellAddress/cellBlockIndex.
In `@packages/docs/src/view/peer-cursor.ts`:
- Around line 80-84: The current logic falls back to 0 when findIndex returns
-1, masking inconsistencies between blockParentMap and cellData.blocks; update
the block index lookup in the peer cursor code (symbols: lb.block.tableData,
cellData, cbi, effectiveCbi, position.blockId, blockParentMap) to log a warning
when cbi < 0 before using effectiveCbi so developers can detect stale or
inconsistent state, keeping the existing fallback to 0 to preserve runtime
behavior.
🪄 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: 1dd97fae-7352-45c2-a186-ceb6d5e5984c
📒 Files selected for processing (20)
docs/design/docs-table-crdt.mddocs/tasks/active/20260329-table-crdt-phase-b-todo.mdpackages/docs/src/index.tspackages/docs/src/model/document.tspackages/docs/src/model/types.tspackages/docs/src/view/editor.tspackages/docs/src/view/find-replace.tspackages/docs/src/view/layout.tspackages/docs/src/view/peer-cursor.tspackages/docs/src/view/selection.tspackages/docs/src/view/table-layout.tspackages/docs/src/view/table-renderer.tspackages/docs/src/view/text-editor.tspackages/docs/test/model/table.test.tspackages/docs/test/view/clipboard.test.tspackages/docs/test/view/pagination.test.tspackages/docs/test/view/peer-cursor.test.tspackages/docs/test/view/table-layout.test.tspackages/docs/test/view/table-selection.test.tspackages/frontend/src/app/docs/docs-table-context-menu.tsx
|
|
||
| - [ ] **Step 2: Remove stale type references** | ||
|
|
||
| Grep for any remaining `cellAddress` or `cellBlockIndex` references outside of the retained `CellAddress` type and `TableCellRange`: |
There was a problem hiding this comment.
Minor: Redundant phrase.
The phrase "outside of" can be simplified to "outside".
📝 Suggested fix
-Grep for any remaining `cellAddress` or `cellBlockIndex` references outside of the retained `CellAddress` type
+Grep for any remaining `cellAddress` or `cellBlockIndex` references outside the retained `CellAddress` type🧰 Tools
🪛 LanguageTool
[style] ~1186-~1186: This phrase is redundant. Consider using “outside”.
Context: ...AddressorcellBlockIndexreferences outside of the retainedCellAddresstype andTa...
(OUTSIDE_OF)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/active/20260329-table-crdt-phase-b-todo.md` at line 1186, Summary:
The phrase "outside of" is wordy and should be simplified to "outside". Edit the
sentence that currently reads "Grep for any remaining `cellAddress` or
`cellBlockIndex` references outside of the retained `CellAddress` type and
`TableCellRange`" to instead read "Grep for any remaining `cellAddress` or
`cellBlockIndex` references outside the retained `CellAddress` type and
`TableCellRange`", leaving the identifiers `cellAddress`, `cellBlockIndex`,
`CellAddress`, and `TableCellRange` unchanged.
toggleList ignored selection range and only toggled the cursor's block. Now handles cell-range, within-cell multi-block, and top-level multi-block selections. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When a top-level selection spans a table block, toggleList and applyBlockStyle now drill into the table's cell blocks instead of modifying the table block itself. Matches Google Docs behavior where bullet/alignment applied across a table affects all cell contents. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
splitBlockInCellInternal lacked the horizontal-rule guard that the top-level splitBlock has. Pressing Enter on a horizontal rule inside a cell would call getStyleAtOffset on empty inlines and throw. Now creates a new paragraph after the rule instead. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
getLinkHrefAtMouse and getLinkAtCursorPosition searched only top-level blocks, missing links inside table cells. Changed to use doc.getBlock() which resolves cell blocks via BlockParentMap. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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/docs/src/view/editor.ts (1)
884-914:⚠️ Potential issue | 🟠 Major
insertLinkdoes not mark cell blocks as dirty.When inserting a link into a cell block,
doc.getBlockIndex(range.anchor.blockId)returns-1because cell blocks are not indoc.document.blocks. The dirty-marking loop (lines 890-898) is skipped entirely, leaving the table block un-repainted.Apply the same
blockParentMaplookup pattern used inapplyStyleandremoveLink:🐛 Proposed fix
doc.applyInlineStyle(range, { href: url }); - const startIdx = doc.getBlockIndex(range.anchor.blockId); - const endIdx = doc.getBlockIndex(range.focus.blockId); - if (startIdx >= 0 && endIdx >= 0) { - const lo = Math.min(startIdx, endIdx); - const hi = Math.max(startIdx, endIdx); - for (let i = lo; i <= hi; i++) { - markDirty(doc.document.blocks[i].id); - } - } + const anchorCI = layout.blockParentMap.get(range.anchor.blockId); + const focusCI = layout.blockParentMap.get(range.focus.blockId); + if (anchorCI) { + markDirty(anchorCI.tableBlockId); + } else if (focusCI) { + markDirty(focusCI.tableBlockId); + } else { + const startIdx = doc.getBlockIndex(range.anchor.blockId); + const endIdx = doc.getBlockIndex(range.focus.blockId); + if (startIdx >= 0 && endIdx >= 0) { + const lo = Math.min(startIdx, endIdx); + const hi = Math.max(startIdx, endIdx); + for (let i = lo; i <= hi; i++) { + markDirty(doc.document.blocks[i].id); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 884 - 914, insertLink fails to mark table cell parent blocks dirty because doc.getBlockIndex(range.anchor.blockId) returns -1 for cell blocks; update insertLink to, when startIdx or endIdx is -1, look up the parent block id via blockParentMap (same pattern used in applyStyle/removeLink) and then markDirty on that parent block id (or its index) so the table/block containing the cell is repainted; ensure you still handle the normal case using doc.getBlockIndex and call markDirty for doc.document.blocks[i].id as before, referencing functions/vars insertLink, doc.getBlockIndex, blockParentMap, applyStyle, removeLink, markDirty, and doc.document.blocks.
🧹 Nitpick comments (1)
packages/docs/src/view/editor.ts (1)
696-729: Consider extracting shared cell-block iteration logic.
applyBlockStyle(lines 696-729) andtoggleList(lines 781-832) both contain nearly identical patterns for iterating blocks within a cell-range selection and within-cell text selection. A shared helper likeforEachBlockInRange(range, layout, doc, callback)would reduce duplication.Also applies to: 781-832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 696 - 729, Both applyBlockStyle and toggleList duplicate the same logic for iterating blocks inside a table-cell selection; extract that into a helper (e.g., forEachBlockInRange(range, layout, doc, callback)) that detects same-cell vs top-level multi-block ranges and calls callback(block, tableId?) for each block in order; update applyBlockStyle and toggleList to call this helper and perform their per-block actions (doc.applyBlockStyle, markDirty, toggling logic) inside the provided callback, preserving behavior for markDirty(tableBlockId) when iterating a cell and markDirty(block.id) for top-level blocks.
🤖 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/docs/src/view/editor.ts`:
- Around line 884-914: insertLink fails to mark table cell parent blocks dirty
because doc.getBlockIndex(range.anchor.blockId) returns -1 for cell blocks;
update insertLink to, when startIdx or endIdx is -1, look up the parent block id
via blockParentMap (same pattern used in applyStyle/removeLink) and then
markDirty on that parent block id (or its index) so the table/block containing
the cell is repainted; ensure you still handle the normal case using
doc.getBlockIndex and call markDirty for doc.document.blocks[i].id as before,
referencing functions/vars insertLink, doc.getBlockIndex, blockParentMap,
applyStyle, removeLink, markDirty, and doc.document.blocks.
---
Nitpick comments:
In `@packages/docs/src/view/editor.ts`:
- Around line 696-729: Both applyBlockStyle and toggleList duplicate the same
logic for iterating blocks inside a table-cell selection; extract that into a
helper (e.g., forEachBlockInRange(range, layout, doc, callback)) that detects
same-cell vs top-level multi-block ranges and calls callback(block, tableId?)
for each block in order; update applyBlockStyle and toggleList to call this
helper and perform their per-block actions (doc.applyBlockStyle, markDirty,
toggling logic) inside the provided callback, preserving behavior for
markDirty(tableBlockId) when iterating a cell and markDirty(block.id) for
top-level blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 024748dd-4f36-4a3e-be63-8a217ec2dda3
📒 Files selected for processing (7)
docs/tasks/README.mddocs/tasks/archive/2026/03/20260329-table-crdt-phase-b-todo.mddocs/tasks/archive/README.mdpackages/docs/src/view/editor.tspackages/frontend/src/app/docs/docs-view.tsxpackages/frontend/src/app/docs/yorkie-doc-store.tspackages/frontend/src/types/users.ts
✅ Files skipped from review due to trivial changes (3)
- docs/tasks/archive/README.md
- packages/frontend/src/types/users.ts
- docs/tasks/README.md
getVisualLineStart/End were short-circuiting to block start/end for cell blocks, ignoring line wrapping. Extended getVisualLineRange to resolve visual lines from the table layout cell data, so Home and End now move to the current visual line boundary. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Collaborator
Author
Suggested squash commit message |
This was referenced Apr 10, 2026
6 tasks
7 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
Replace JSON-stringified
tableDatawith Yorkie Tree node hierarchy and unify the editing pipeline so table cell blocks are addressed byblockIdalone.Phase A: Data model + Store
TableCellfromInline[]toBlock[]containers (multi-paragraph cells)row → cell → block → inline → text)Phase B: Unified editing pipeline
blockId → { tableBlockId, rowIndex, colIndex }) built during layoutgetBlock,insertText,deleteText,splitBlock,mergeBlocks,applyInlineStyle,applyBlockStyle,setBlockTypehandle both top-level and cell blockscellAddress/cellBlockIndexfromDocPositionand all 8*InCellmethods (~500 lines deleted)if (cellAddress)branches withBlockParentMapqueriesBug fixes
Test plan
pnpm verify:fastpassespnpm verify:selfpasses🤖 Generated with Claude Code