docs/design/docs-tables.md (2)

413-416: Minor: Update file location claim for accuracy.

Lines 414-415 state "All changes are in packages/docs/src/view/text-editor.ts except (1)", but change area #6 (lines 569-586) also touches packages/docs/src/view/selection.ts. Consider updating to: "All changes are in packages/docs/src/view/text-editor.ts except (1) and (6) which touch packages/docs/src/view/selection.ts."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/docs-tables.md` around lines 413 - 416, Update the sentence that
currently reads "All changes are in `packages/docs/src/view/text-editor.ts`
except (1)" to mention that change area `#6` also touches selection.ts;
specifically edit the paragraph around the list of seven areas (the line
referencing "except (1)") to: "All changes are in
`packages/docs/src/view/text-editor.ts` except (1) and (6) which touch
`packages/docs/src/view/selection.ts`", ensuring the updated text appears in the
same block that lists the seven change areas.

---

40-42: Add language specifiers to fenced code blocks.

The fenced code blocks on lines 40-42 and 59-61 should specify a language for better syntax highlighting and accessibility. Since these represent structural diagrams, consider using text or plaintext:

📝 Suggested fix

-```
+```text
 Document → Block[] → Block.type = 'table', Block.tableData = TableData

Apply the same pattern to line 59:
```diff
-```
+```text
 Document → Block[] → Block.type = 'table' → cells[] → Block[]

</details>


As per coding guidelines, this improves documentation quality in `docs/design/` markdown files.

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/design/docs-tables.md around lines 40 - 42, Update the two fenced code
blocks that show the structural diagrams so they include a language specifier
for syntax highlighting (use "text"); specifically change the blocks containing
"Document → Block[] → Block.type = 'table', Block.tableData = TableData" and
"Document → Block[] → Block.type = 'table' → cells[] → Block[]" to use text instead of plain so the diagrams render with the suggested highlighting.

</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 @packages/docs/src/view/doc-canvas.ts:

• Around line 170-198: The current page-slice loop for table blocks only renders
  rows between startRowIndex and endRowIndex which drops cells whose rowspan
  begins above startRowIndex; update the logic in the block that computes
  startRowIndex/endRowIndex (the PageLine scan using pl.blockIndex and plIndex) to
  detect table cells in lb.block.tableData with rowSpan that start before
  startRowIndex but extend into the current slice, and either (A) expand
  startRowIndex to include the originating row so the whole spanning region is
  rendered on one page, or (B) pass metadata into renderTable (call site here and
  its signature) indicating which cells have span origins above the slice (e.g.,
  spanOriginRow < startRowIndex) so renderTable can draw the continued portion
  (background/borders/text) for those spanning cells; adjust
  lb.layoutTable.rowYOffsets usage when computing tableOriginY so renderTable can
  compute clipped geometry correctly.

In @packages/docs/src/view/editor.ts:

• Around line 924-930: After deleting a table row/column the caret's cellAddress
  can point to a non-existent cell; update the caret to a valid location after
  calling doc.deleteRow/doc.deleteColumn. Specifically, after docStore.snapshot()
  and doc.deleteRow(cursor.position.blockId, ca.rowIndex) (and likewise for
  deleteColumn), compute a new cellAddress by clamping the rowIndex/columnIndex to
  the table's new bounds (e.g., max(0, min(oldIndex, newRowCount-1))) and assign
  it to cursor.position.cellAddress; if the table has no rows/columns left, move
  the caret out of the table block (mark isInTable() false) to a sensible fallback
  position (e.g., start of the block or next block). Then call invalidateLayout()
  and render(). Use the existing symbols cursor.position.cellAddress,
  doc.deleteRow, doc.deleteColumn, isInTable(), invalidateLayout(), render(), and
  docStore.snapshot() to locate and apply this fix.
• Around line 949-954: mergeTableCells currently merges cells but doesn't update
  the caret, causing it to disappear if the current position became a merged
  placeholder; after calling doc.mergeCells(...) in mergeTableCells, compute the
  surviving/top-left cell address from the provided CellRange (e.g., range.start
  row/col) and update the cursor position to that surviving cell (use the existing
  cursor API such as cursor.position = { blockId: , ... } or
  cursor.moveTo(...) depending on available helpers) so resolvePositionPixel() can
  find a valid pixel position; keep the snapshot/invalidateLayout/render calls
  as-is and perform the cursor update immediately after doc.mergeCells(...) and
  before invalidateLayout()/render().

In @packages/docs/src/view/selection.ts:

• Around line 119-130: The current same-cell branch uses a single rect from
  startPixel to endPixel which breaks for wrapped text; instead detect
  per-visual-line positions from resolvePositionPixel (use any line/fragment
  indices or metrics available on startPixel/endPixel or the cell's layout in
  paginatedLayout/layout) and emit one rectangle per wrapped line: first line from
  startPixel.x to lineEndX, middle full-width lines across the cell, and last line
  from lineStartX to endPixel.x; iterate from start line to end line and construct
  rects using each line's y, height, and correct x/width so negative widths are
  avoided (update the code around resolvePositionPixel/startPixel/endPixel
  handling to produce multiple rects).
• Around line 23-35: The current same-cell branch returns null for ranges that
  cross cells or drops cellAddress when only one endpoint has it; instead,
  normalize cross-cell and mixed-cell/block ranges by ordering endpoints across
  blockId, then cellAddress (rowIndex, colIndex), then offset, and return {start,
  end} preserving each endpoint's cellAddress when present (compare range.anchor
  vs range.focus using blockId, cellAddress.rowIndex/colIndex, then offset to
  decide order). Also ensure the case where only one endpoint has cellAddress does
  not fall through to the block-only ordering that discards cellAddress—preserve
  cellAddress on that endpoint in the returned start/end.

---

Nitpick comments:
In @docs/design/docs-tables.md:

• Around line 413-416: Update the sentence that currently reads "All changes are
  in packages/docs/src/view/text-editor.ts except (1)" to mention that change
  area #6 also touches selection.ts; specifically edit the paragraph around the
  list of seven areas (the line referencing "except (1)") to: "All changes are in
  packages/docs/src/view/text-editor.ts except (1) and (6) which touch
  packages/docs/src/view/selection.ts", ensuring the updated text appears in the
  same block that lists the seven change areas.
• Around line 40-42: Update the two fenced code blocks that show the structural
  diagrams so they include a language specifier for syntax highlighting (use
  "text"); specifically change the blocks containing "Document → Block[] →
  Block.type = 'table', Block.tableData = TableData" and "Document → Block[] →
  Block.type = 'table' → cells[] → Block[]" to use text instead of plain so
  the diagrams render with the suggested highlighting.

</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**: `66950e26-6960-4a31-a6ef-4245636df3da`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between e4074069d3528226f13143793c733b643ecdf102 and 68137cb3ca742451ba153c7bab038003f90cc840.

</details>

<details>
<summary>📒 Files selected for processing (11)</summary>

* `docs/design/README.md`
* `docs/design/docs-tables.md`
* `packages/docs/src/model/document.ts`
* `packages/docs/src/view/doc-canvas.ts`
* `packages/docs/src/view/editor.ts`
* `packages/docs/src/view/selection.ts`
* `packages/docs/src/view/table-renderer.ts`
* `packages/docs/src/view/text-editor.ts`
* `packages/docs/test/view/table-selection.test.ts`
* `packages/frontend/src/app/docs/docs-table-context-menu.tsx`
* `packages/frontend/src/app/docs/yorkie-doc-store.ts`

</details>

<details>
<summary>✅ Files skipped from review due to trivial changes (2)</summary>

* docs/design/README.md
* packages/docs/src/model/document.ts

</details>

<details>
<summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary>

* packages/frontend/src/app/docs/yorkie-doc-store.ts
* packages/docs/src/view/text-editor.ts

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

docs/tasks/active/20260329-table-crdt-phase-a-todo.md (3)

669-693: Reuse getCellText helper instead of duplicating logic.

The getCellText and getCellTextLength implementations in text-editor.ts duplicate the text extraction logic already defined in types.ts (Task 1, Step 3, lines 96-98).

Since getCellText(cell: TableCell) is being exported from types.ts (Task 7, Step 1), import and reuse it:

♻️ Refactor to eliminate duplication

+import { getCellText } from '../model/types.js';

   private getCellTextLength(blockId: string, cell: CellAddress): number {
     const block = this.doc.getBlock(blockId);
     if (!block.tableData) return 0;
     const row = block.tableData.rows[cell.rowIndex];
     if (!row) return 0;
     const tc = row.cells[cell.colIndex];
     if (!tc) return 0;
-    return tc.blocks.flatMap(b => b.inlines).reduce((s, i) => s + i.text.length, 0);
+    return getCellText(tc).length;
   }

   private getCellText(blockId: string, cell: CellAddress): string {
     const block = this.doc.getBlock(blockId);
     if (!block.tableData) return '';
     const row = block.tableData.rows[cell.rowIndex];
     if (!row) return '';
     const tc = row.cells[cell.colIndex];
     if (!tc) return '';
-    return tc.blocks.flatMap(b => b.inlines).map(i => i.text).join('');
+    return getCellText(tc);
   }

Or rename the private methods to getCellTextById / getCellTextLengthById to clarify they do lookup + extraction.

🤖 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-a-todo.md` around lines 669 -
693, The private TextEditor methods getCellText and getCellTextLength duplicate
extraction logic — import the exported getCellText(TableCell) from types.ts and
reuse it: in TextEditor locate the two methods (references: getCellText,
getCellTextLength, CellAddress, this.doc.getBlock) and change them to fetch the
block/row/cell (via this.doc.getBlock and row.cells[cell.colIndex]) to obtain
the TableCell, then call the imported getCellText to return text and compute
length (or return its .length), or alternatively rename the current methods to
getCellTextById/getCellTextLengthById to clarify they perform lookup+delegation
before invoking the shared types.ts getCellText.

---

505-517: Validate that parsed tables have at least one column.

Line 509 filters cols values to remove NaN entries. If attrs.cols is malformed or empty, columnWidths could become an empty array, which would break table rendering (tables must have ≥1 column).

🛡️ Add validation or fallback

     const cols = (attrs.cols ?? '').split(',').map(Number).filter(n => !isNaN(n));
+    if (cols.length === 0) {
+      // Fallback: single column if cols attribute is malformed
+      cols.push(1.0);
+    }
     return {

Alternatively, reject the block entirely if cols.length === 0 and log a warning, but a fallback is more resilient to malformed data.

🤖 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-a-todo.md` around lines 505 -
517, The table-parsing code can yield an empty columnWidths array when
attrs.cols is missing/malformed; update the block handling (the blockType ===
'table' branch where cols is computed from attrs.cols and tableData is returned)
to validate cols and ensure at least one column: if the filtered cols array is
empty, either set a safe default (e.g., [100] or [1]) before constructing
tableData or reject the block by returning null/undefined and logging a warning
via the existing logging mechanism; make the change where cols is created and
before returning the object so tableData.columnWidths always has length ≥ 1.

---

437-453: Add validation for border style parsing.

parseBorderStyle returns undefined for invalid input (line 440), and the style field is cast as 'solid' | 'none' without validation. If the serialized border has an invalid style value (e.g., "1,dashed,#000"), this will silently pass through.

🛡️ Recommended fix with validation

 function parseBorderStyle(value: string): BorderStyle | undefined {
   const parts = value.split(',');
   if (parts.length !== 3) return undefined;
+  const style = parts[1];
+  if (style !== 'solid' && style !== 'none') return undefined;
-  return { width: Number(parts[0]), style: parts[1] as 'solid' | 'none', color: parts[2] };
+  return { width: Number(parts[0]), style: style as 'solid' | 'none', color: parts[2] };
 }

This ensures only valid border styles are parsed. Malformed borders are treated as undefined (no border), which is a safe fallback.

🤖 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-a-todo.md` around lines 437 -
453, The parseBorderStyle function lacks validation for the style token and
numeric width and can silently accept invalid values; update parseBorderStyle to
validate that parts.length === 3, the width parses to a finite number, the style
token is exactly 'solid' or 'none', and the color is non-empty—otherwise return
undefined; then in parseCellStyle only assign
borderTop/borderBottom/borderLeft/borderRight when parseBorderStyle(...) returns
a defined BorderStyle (do not cast strings to 'solid'|'none' without checking).
Use the function names parseBorderStyle and parseCellStyle and types
BorderStyle/CellStyle to locate and change the logic.

docs/design/docs-table-crdt.md (3)

206-216: Clarify the rationale for Enter key behavior.

The navigation table states that Enter inserts a new paragraph block within the cell rather than moving to the next cell/row. This is a departure from spreadsheet-style UX (where Enter typically moves down) and aligns with word-processor behavior.

Consider expanding the explanation to note:

• This matches Google Docs table behavior (multi-paragraph cells)
• Users can still navigate cells with Tab/Shift+Tab or Arrow keys
• This is intentional to support rich cell content (lists, headings, multiple paragraphs)

The current note is sufficient but a brief rationale strengthens the design.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/docs-table-crdt.md` around lines 206 - 216, Update the Enter key
row in the navigation table to include a brief rationale: state that Enter
creates a new paragraph inside the current cell (matching word-processor
behavior and Google Docs tables to support multi-paragraph cells), note that
users still navigate between cells using Tab/Shift+Tab or Arrow keys (which rely
on BlockParentMap for cell context), and explicitly call out this choice is
intentional to support rich cell content like lists, headings, and multiple
paragraphs.

---

292-308: Consider adding a concrete editByPath example.

The Granular Updates table clearly maps operations to Yorkie Tree calls, but editByPath usage is Yorkie-specific and would benefit from a code example. For instance:

// Example: Insert row via editByPath
doc.update((root) => {
  const tableNode = root.getElementByID(blockId);
  tableNode.insertAfter(rowIndex, {
    type: 'row',
    attributes: {},
    children: /* cell nodes */
  });
});

This helps bridge the gap between the design and Yorkie API usage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/docs-table-crdt.md` around lines 292 - 308, Add a concrete Yorkie
editByPath example to the "Granular Updates" section: show a short, explicit
sequence using doc.update, locating the table node (e.g.,
getElementByID(blockId) or root lookup), then calling editByPath/insertAfter to
insert a row node with cell children and/or editByPath on inline/text nodes for
cell edits; reference the existing symbols updateBlock, DocStore, editByPath,
getElementByID, insertAfter so readers can map the prose to the code paths and
clarify where DocStore should emit these granular tree edits versus wholesale
updateBlock replacement.

---

183-204: Recommend adding BlockParentMap construction example.

The BlockParentMap concept is well-defined, but the design would benefit from a code snippet showing how it's built during layout computation. Consider adding:

// Example: Building BlockParentMap during table layout
function buildBlockParentMap(tableData: TableData, tableBlockId: string): Map<string, BlockCellInfo> {
  const map = new Map<string, BlockCellInfo>();
  for (let r = 0; r < tableData.rows.length; r++) {
    for (let c = 0; c < tableData.rows[r].cells.length; c++) {
      const cell = tableData.rows[r].cells[c];
      for (const block of cell.blocks) {
        map.set(block.id, { tableBlockId, rowIndex: r, colIndex: c });
      }
    }
  }
  return map;
}

This helps clarify when/how the cache is populated and invalidated.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/docs-table-crdt.md` around lines 183 - 204, Add a short example
showing how BlockParentMap is constructed and cached during layout computation:
describe a function (e.g. buildBlockParentMap) that walks the TableData
rows/cells and maps each cell block.id to a BlockCellInfo { tableBlockId,
rowIndex, colIndex }, and mention storing the resulting Map on DocumentLayout
(invalidate/rebuild when the table block is dirty). Reference BlockParentMap,
BlockCellInfo, buildBlockParentMap and DocumentLayout so readers know where to
place and invalidate the cache.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes