packages/docs/src/view/image-selection-overlay.ts (1)

386-451: Consider extracting shared logic to reduce duplication.

The collectNestedTableImageRects function duplicates much of the logic from collectTableCellImageRects (merged cell filtering, row-span handling, vertical alignment checks, block boundary tracking, image rect collection). This increases maintenance burden if the coordinate math or filtering logic needs to change.

A future refactor could extract the common per-cell image collection logic into a shared helper, with the only difference being the coordinate origin calculation.

The implementation is correct and the coordinate math properly mirrors renderTableContent. The recursive call at lines 429-436 correctly handles arbitrary nesting depth.

🤖 Prompt for AI Agents

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

In `@packages/docs/src/view/image-selection-overlay.ts` around lines 386 - 451,
collectNestedTableImageRects duplicates much of the per-cell logic in
collectTableCellImageRects (merged cell check, rowSpan handling, verticalAlign
check, blockBoundary/currentBlockIdx logic, and image rect calculation); extract
that shared per-cell image-collection logic into a helper (e.g.,
collectImageRectsForCell) that accepts the cell data, layoutCell, origin offsets
(cellX, cellY or padding-adjusted origin), and the out Map, then call that
helper from both collectTableCellImageRects and collectNestedTableImageRects and
keep the recursive nested-table call inside the helper (or invoke the helper
before recursing) so coordinate math is centralized and duplication removed.

docs/tasks/active/20260413-nested-tables-todo.md (2)

319-330: Test assumes nested table is at index 1 — verify createTableCell() behavior.

Line 321 accesses outer.rows[0].cells[0].blocks[1] assuming the nested table is the second block. This relies on createTableCell() creating a default paragraph at index 0. If createTableCell() creates an empty blocks array, this test will fail with an undefined access.

Consider either:

1. Explicitly documenting this assumption in a comment, or
2. Using a more robust lookup: cell.blocks.find(b => b.type === 'table')

♻️ Suggested defensive approach

  it('should include inner table cell blocks in blockParentMap', () => {
    const { outer } = makeNestedTableData();
-   const innerTable = outer.rows[0].cells[0].blocks[1]; // index 1 = nested table
+   const innerTable = outer.rows[0].cells[0].blocks.find(b => b.type === 'table');
+   expect(innerTable).toBeDefined();
    const innerCellBlockId = innerTable.tableData!.rows[0].cells[0].blocks[0].id;

🤖 Prompt for AI Agents

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

In `@docs/tasks/active/20260413-nested-tables-todo.md` around lines 319 - 330, The
test in makeNestedTableData assumes the nested table sits at
outer.rows[0].cells[0].blocks[1], which breaks if createTableCell() no longer
inserts a default paragraph; update the test to find the nested table
defensively by searching the cell's blocks (e.g., use
outer.rows[0].cells[0].blocks.find(b => b.type === 'table') or similar) instead
of indexing, and keep references to computeTableLayout, makeNestedTableData, and
innerTable.tableData access unchanged so the rest of the test still validates
blockParentMap correctly.

---

1021-1030: Manual blockParentMap extension for 3-level nesting is fragile.

The test manually extends the map for level 2 (lines 1024-1030), but this duplicates the logic that buildParentMapRecursive should handle. If buildParentMapRecursive doesn't support arbitrary depth, the test is masking a real limitation.

Consider either:

1. Making buildParentMapRecursive truly recursive (handle any depth), or
2. Adding a comment explaining why manual extension is necessary for the test

🤖 Prompt for AI Agents

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

In `@docs/tasks/active/20260413-nested-tables-todo.md` around lines 1021 - 1030,
The test is manually extending map for a 3rd-level table which hides a
limitation in buildParentMapRecursive; update buildParentMapRecursive so it
truly walks nested tables to arbitrary depth: traverse document nodes, when
encountering a table block iterate its rows/cells and add each cell block id to
the parent map with tableBlockId/rowIndex/colIndex, and recurse into any nested
table blocks found (so the current manual extension around innermostTable is no
longer needed); if you choose not to make it recursive, add a clear comment
above the manual extension explaining why buildParentMapRecursive cannot handle
depth >2 and reference buildParentMapRecursive and innermostTable so future
readers know why the map is augmented manually.

packages/docs/test/view/nested-table-layout.test.ts (1)

45-54: Same fragility as the todo.md — hardcoded blocks[1] index.

Line 47 assumes the nested table is at index 1 in cell.blocks. If createTableCell() doesn't create a default paragraph at index 0, or if makeNestedTableData() pushes the table differently, this will break.

♻️ Suggested defensive lookup

  it('should include inner table cell blocks in blockParentMap', () => {
    const { outer } = makeNestedTableData();
-   const innerTable = outer.rows[0].cells[0].blocks[1]; // index 1 = nested table
+   const innerTable = outer.rows[0].cells[0].blocks.find(b => b.type === 'table');
+   if (!innerTable) throw new Error('Expected nested table in cell');
    const innerCellBlockId = innerTable.tableData!.rows[0].cells[0].blocks[0].id;

🤖 Prompt for AI Agents

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

In `@packages/docs/test/view/nested-table-layout.test.ts` around lines 45 - 54,
The test is fragile because it assumes the nested table lives at blocks[1];
instead, locate the nested table defensively by searching the cell's blocks for
the block with type 'table' (or where block.tableData is defined) rather than
using a hardcoded index; in the test that calls makeNestedTableData() and then
references outer.rows[0].cells[0].blocks[1], replace that lookup with a find on
outer.rows[0].cells[0].blocks to get innerTable (e.g., block => block.type ===
'table' || Boolean(block.tableData)), then extract innerCellBlockId from
innerTable.tableData.rows[0].cells[0].blocks[0].id and proceed with the same
assertions on computeTableLayout and layout.blockParentMap.

packages/docs/src/view/text-editor.ts (4)

3556-3561: Depth-2 nesting limitation in click resolution.

Click-to-character resolution falls back to offset 0 for tables nested 3+ levels deep. While the fallback is graceful, users may find cursor placement imprecise in deeply nested scenarios.

Consider extracting this resolution logic into a recursive helper if deeper nesting support is needed in the future.

🤖 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 3556 - 3561, The current
click-to-character resolution in the block lookup falls back to returning {
blockId: innerCellData.blocks[0].id, offset: 0 } when
innerTargetLine?.nestedTable is true, which forces offset 0 for depth-2+ nested
tables; refactor by extracting this logic into a recursive helper (e.g.,
resolveNestedClick or similar) that takes innerCellLayout, innerLineIdx, and
innerCellData and walks nestedTable chains until it finds a non-nested line or
computes a proper offset, then return the found blockId and computed offset
instead of always returning offset 0; update callers that reference the inline
fallback to use the new helper so deeper nesting is handled consistently.

---

1877-1892: Column fallback to 0 may feel jarring for users.

When entering a nested table from a column index that exceeds the nested table's column count, the cursor jumps to column 0 rather than clamping to the last column. For example, navigating down from column 3 into a 2-column nested table lands at column 0 instead of column 1.

Consider clamping to Math.min(arrowCellInfo.colIndex, td.columnWidths.length - 1) for more intuitive behavior:

♻️ Suggested fix for column clamping

-          const lastCell = td.rows[lastRow].cells[arrowCellInfo.colIndex < td.columnWidths.length ? arrowCellInfo.colIndex : 0];
+          const col = Math.min(arrowCellInfo.colIndex, td.columnWidths.length - 1);
+          const lastCell = td.rows[lastRow].cells[col];

Apply similar changes to lines 1902, 1957, 1978-1979, and 2005-2006.

🤖 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 1877 - 1892, The cursor
column fallback currently uses "arrowCellInfo.colIndex < td.columnWidths.length
? arrowCellInfo.colIndex : 0" which jumps to column 0 when the incoming column
exceeds the nested table's columns; change this to clamp the column to the last
valid index using Math.min(arrowCellInfo.colIndex, td.columnWidths.length - 1)
when computing lastCell (and similarly adjust the other occurrences noted), so
lastCell = td.rows[lastRow].cells[ Math.min(arrowCellInfo.colIndex,
td.columnWidths.length - 1) ] and ensure newPos offset logic remains the same;
update the same pattern wherever you handle nested table entry (the other
occurrences the reviewer listed).

---

3686-3691: Cursor state modification before recursive call is subtle but correct.

The temporary cursor.moveTo before recursion intentionally shifts the navigation context to the parent cell. This works correctly but relies on getCellInfo using the cursor's current blockId for table context resolution.

A clarifying comment explaining this context-switching pattern would aid future maintainers.

📝 Suggested comment addition

       // No more blocks in parent cell — navigate to the next cell in the
       // outer table. Temporarily place the cursor on the first block of
       // the parent cell so getCellInfo resolves to the outer table context.
+      // This context switch allows the recursive call to navigate the
+      // parent table rather than the nested table we're exiting.
       this.cursor.moveTo({ blockId: parentCell.blocks[0].id, offset: 0 });
       return this.moveToNextCell(addRowAtEnd);

🤖 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 3686 - 3691, Add a
clarifying inline comment before the cursor context-switch to explain why we
call this.cursor.moveTo({ blockId: parentCell.blocks[0].id, offset: 0 }) prior
to the recursive this.moveToNextCell(addRowAtEnd) call: state that the temporary
move sets the cursor's blockId so getCellInfo resolves the outer table context
(not the current nested cell), and that this is intentional and temporary to
allow moveToNextCell to operate on the parent cell; reference the symbols
cursor.moveTo, parentCell.blocks[0].id, getCellInfo, and moveToNextCell to make
the rationale explicit for future maintainers.

---

2776-2784: Safe nested table entry assuming table invariants hold.

The direct access to td.rows[lastRow].cells[lastCol] and lastCell.blocks[lastCell.blocks.length - 1] relies on the invariant that tables always have at least one row, column, and block per cell. This is enforced by createTableBlock, but a defensive check could prevent cryptic errors if invariants are violated by external mutations.

🤖 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 2776 - 2784, The code
assumes table invariants in the nested-table branch (variables td, lastRow,
lastCol, lastCell, lastCellBlock) and directly accesses
td.rows[lastRow].cells[lastCol] and lastCell.blocks[lastCell.blocks.length - 1];
add defensive checks to ensure td.rows.length > 0, td.columnWidths.length > 0,
td.rows[lastRow] and td.rows[lastRow].cells[lastCol] exist, and that
lastCell.blocks.length > 0 before reading the final block; if any check fails,
fall back to a safe value (e.g., return { blockId: prevBlock.id, offset: 0 } or
similar) and otherwise return the intended { blockId: lastCellBlock.id, offset:
getBlockTextLength(lastCellBlock) } so getBlockTextLength is only called on a
valid block.

packages/docs/src/model/document.ts (1)

633-637: Cell invariant correctly maintained, but code can be more efficient.

The code ensures the cell retains at least one block after table deletion. However, using createTableCell().blocks[0] creates a full cell object just to extract one block. Since createEmptyBlock() is already imported in this file, use it directly for better efficiency:

♻️ Suggested optimization

     // Ensure cell still has at least one block
     if (parentCell.blocks.length === 0) {
-      const emptyBlock = createTableCell().blocks[0];
+      const emptyBlock = createEmptyBlock();
       parentCell.blocks.push(emptyBlock);
     }

🤖 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 633 - 637, The code
maintains the cell invariant by pushing a block when parentCell.blocks is empty,
but it currently instantiates a full cell via createTableCell().blocks[0];
replace that with the already-imported createEmptyBlock() to avoid creating an
unnecessary cell object—locate the conditional that checks
parentCell.blocks.length === 0 and push createEmptyBlock() onto
parentCell.blocks instead of createTableCell().blocks[0].

Fix all unresolved CodeRabbit comments on this PR:

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