fix(document-api): delete table cell fix (#2209)

* fix(document-api): delete table cell fix

* fix(document-api): remove double-mapping of rowEndPos in shiftLeft vMerge fallback

Author: harbournick

packages/super-editor/src/document-api-adapters/tables-adapter.regressions.test.ts

@@ -330,6 +330,137 @@ describe('tables-adapter regressions', () => {
     expect(tr.insert).toHaveBeenCalledWith(expectedInsertPos, expect.anything());
   });
 
+  it('deletes shiftLeft cells without appending a trailing replacement cell', () => {
+    const editor = makeTableEditor();
+    const tr = editor.state.tr as unknown as {
+      delete: ReturnType<typeof vi.fn>;
+      insert: ReturnType<typeof vi.fn>;
+      setNodeMarkup: ReturnType<typeof vi.fn>;
+    };
+    const tableNode = editor.state.doc.nodeAt(0) as ProseMirrorNode;
+    const targetCellOffset = TableMap.get(tableNode).map[0]!;
+    const targetCellNode = tableNode.nodeAt(targetCellOffset) as ProseMirrorNode;
+    const expectedStart = 1 + targetCellOffset;
+    const expectedEnd = expectedStart + targetCellNode.nodeSize;
+
+    const result = tablesDeleteCellAdapter(editor, { nodeId: 'cell-1', mode: 'shiftLeft' });
+    expect(result.success).toBe(true);
+    expect(tr.delete).toHaveBeenCalledWith(expectedStart, expectedEnd);
+    expect(tr.insert).not.toHaveBeenCalled();
+    expect(tr.setNodeMarkup).toHaveBeenCalledWith(
+      expect.any(Number),
+      null,
+      expect.objectContaining({
+        colspan: 2,
+      }),
+    );
+  });
+
+  it('deletes the row trailing cell for shiftLeft without appending a replacement cell', () => {
+    const editor = makeTableEditor();
+    const tr = editor.state.tr as unknown as {
+      delete: ReturnType<typeof vi.fn>;
+      insert: ReturnType<typeof vi.fn>;
+      setNodeMarkup: ReturnType<typeof vi.fn>;
+    };
+    const tableNode = editor.state.doc.nodeAt(0) as ProseMirrorNode;
+    const targetCellOffset = TableMap.get(tableNode).map[1]!;
+    const targetCellNode = tableNode.nodeAt(targetCellOffset) as ProseMirrorNode;
+    const expectedStart = 1 + targetCellOffset;
+    const expectedEnd = expectedStart + targetCellNode.nodeSize;
+
+    const result = tablesDeleteCellAdapter(editor, { nodeId: 'cell-2', mode: 'shiftLeft' });
+    expect(result.success).toBe(true);
+    expect(tr.delete).toHaveBeenCalledWith(expectedStart, expectedEnd);
+    expect(tr.insert).not.toHaveBeenCalled();
+    expect(tr.setNodeMarkup).toHaveBeenCalledWith(
+      expect.any(Number),
+      null,
+      expect.objectContaining({
+        colspan: 2,
+      }),
+    );
+  });
+
+  it('falls back to trailing replacement cell when shiftLeft would widen a vertically merged trailing cell', () => {
+    const editor = makeTableEditor();
+    const tr = editor.state.tr as unknown as {
+      delete: ReturnType<typeof vi.fn>;
+      insert: ReturnType<typeof vi.fn>;
+      setNodeMarkup: ReturnType<typeof vi.fn>;
+    };
+    const tableNode = editor.state.doc.nodeAt(0) as ProseMirrorNode;
+    const firstRow = tableNode.child(0) as ProseMirrorNode;
+    const trailingCell = firstRow.child(1) as unknown as { attrs: Record<string, unknown> };
+    trailingCell.attrs.rowspan = 2;
+    trailingCell.attrs.tableCellProperties = { vMerge: 'restart' };
+
+    const result = tablesDeleteCellAdapter(editor, { nodeId: 'cell-1', mode: 'shiftLeft' });
+    expect(result.success).toBe(true);
+    expect(tr.insert).toHaveBeenCalledWith(expect.any(Number), expect.anything());
+    expect(tr.setNodeMarkup).not.toHaveBeenCalled();
+  });
+
+  it('shiftLeft vMerge fallback inserts at the post-delete row end without double-mapping', () => {
+    // Regression: rowEndPos was computed from the post-delete doc (tr.doc) but then
+    // passed through tr.mapping.map() which maps old→new, double-shifting the position.
+    const editor = makeTableEditor();
+
+    const tableNode = editor.state.doc.nodeAt(0) as ProseMirrorNode;
+    const firstRow = tableNode.child(0) as ProseMirrorNode;
+    const trailingCell = firstRow.child(1) as unknown as { attrs: Record<string, unknown> };
+    trailingCell.attrs.rowspan = 2;
+    trailingCell.attrs.tableCellProperties = { vMerge: 'restart' };
+
+    const cell1 = firstRow.child(0);
+    const deletionStart = 2; // absolute position of cell-1
+    const deletionSize = cell1.nodeSize; // 9
+
+    // Build post-delete table: row 0 only contains the vMerge cell.
+    const postDeleteRow0 = createNode('tableRow', [firstRow.child(1)], {
+      attrs: { ...(firstRow.attrs as Record<string, unknown>) },
+      isBlock: true,
+      inlineContent: false,
+    });
+    const postDeleteTable = createNode('table', [postDeleteRow0, tableNode.child(1)], {
+      attrs: { ...(tableNode.attrs as Record<string, unknown>) },
+      isBlock: true,
+      inlineContent: false,
+    });
+    const postDeleteDoc = createNode('doc', [postDeleteTable], { isBlock: false });
+
+    const trObj = editor.state.tr as unknown as {
+      delete: ReturnType<typeof vi.fn>;
+      insert: ReturnType<typeof vi.fn>;
+      mapping: { map: (p: number) => number; maps: unknown[]; slice: () => { map: (p: number) => number } };
+      doc: ProseMirrorNode;
+    };
+
+    // Swap tr.doc to the post-delete document when delete is called.
+    trObj.delete = vi.fn(() => {
+      trObj.doc = postDeleteDoc;
+      return trObj;
+    });
+
+    // Simulate real deletion mapping: positions at or after the deleted range shift left.
+    trObj.mapping.map = (pos: number) => {
+      if (pos < deletionStart) return pos;
+      if (pos < deletionStart + deletionSize) return deletionStart;
+      return pos - deletionSize;
+    };
+
+    const result = tablesDeleteCellAdapter(editor, { nodeId: 'cell-1', mode: 'shiftLeft' });
+    expect(result.success).toBe(true);
+    expect(trObj.insert).toHaveBeenCalled();
+
+    // Post-delete row 0 nodeSize = 2 + cell-2 size (9) = 11.
+    // rowEndPos = tablePos(0) + 1 + 11 = 12.
+    // Correct insert = 12 - 1 = 11 (just inside the row).
+    // Old buggy code: tr.mapping.map(11) = 11 - 9 = 2 — wrong position!
+    const insertPos = trObj.insert.mock.calls[0]![0];
+    expect(insertPos).toBe(11);
+  });
+
   it('keeps table grid widths in sync when distributing columns', () => {
     const editor = makeTableEditor();
     const tr = editor.state.tr as unknown as { setNodeMarkup: ReturnType<typeof vi.fn> };

packages/super-editor/src/document-api-adapters/tables-adapter.ts

@@ -1742,8 +1742,9 @@ export function tablesInsertCellAdapter(
 /**
  * tables.deleteCell — delete a cell at a resolved position, shifting remaining cells.
  *
- * `shiftLeft`: removes the target cell and appends a new empty cell at the end of
- * the row to maintain column count.
+ * `shiftLeft`: removes the target cell and shifts remaining cells in the row left.
+ * This reduces the row cell count by one and avoids a synthetic trailing cell unless
+ * widening the remaining trailing cell would conflict with vertical merges.
  *
  * `shiftUp`: removes the target cell and appends a new empty cell at the same column
  * in the last row to maintain row count.
@@ -1757,6 +1758,15 @@ export function tablesDeleteCellAdapter(
 
   const resolved = resolveCellLocator(editor, input, 'tables.deleteCell');
   const { table, cellPos, cellNode, rowIndex, columnIndex } = resolved;
+  const row = table.candidate.node.child(rowIndex);
+  const deletedColspan = Math.max(1, ((cellNode.attrs as Record<string, unknown>).colspan as number) || 1);
+  const deletedColwidth = Array.isArray((cellNode.attrs as Record<string, unknown>).colwidth)
+    ? [...((cellNode.attrs as Record<string, unknown>).colwidth as number[])]
+    : null;
+
+  if (input.mode === 'shiftLeft' && row.childCount <= 1) {
+    return toTableFailure('NO_OP', 'Cannot shift-left delete the last remaining cell in a row.');
+  }
 
   if (options?.dryRun) {
     return buildTableSuccess(table.address);
@@ -1774,17 +1784,61 @@ export function tablesDeleteCellAdapter(
     tr.delete(cellPos, cellPos + cellNode.nodeSize);
 
     if (input.mode === 'shiftLeft') {
-      // Append a new empty cell at the end of this row.
-      const row = tableNode.child(rowIndex);
-      const rowEnd = tr.mapping.map(tableStart + map.positionAt(rowIndex, map.width - 1, tableNode));
-      const lastCell = row.child(row.childCount - 1);
-      const insertPos = tr.mapping.map(cellPos + cellNode.nodeSize + lastCell.nodeSize - cellNode.nodeSize);
-      // Find the end of the row in the mapped doc — simpler: compute row end position.
-      let rowEndPos = table.candidate.pos + 1;
-      for (let i = 0; i <= rowIndex; i++) rowEndPos += tableNode.child(i).nodeSize;
-      const mappedRowEnd = tr.mapping.map(rowEndPos - 1); // -1 to be inside the row closing tag
-      const newCell = schema.nodes.tableCell.createAndFill()!;
-      tr.insert(mappedRowEnd, newCell);
+      // Prefer preserving fewer visual cells by widening the new trailing cell.
+      // Fall back to a trailing replacement cell when merged geometry would become invalid.
+      const currentTableNode = tr.doc.nodeAt(tablePos);
+      if (!currentTableNode || currentTableNode.type.name !== 'table') {
+        return toTableFailure('INVALID_TARGET', 'Cell deletion could not locate the updated table.');
+      }
+
+      const currentRow = currentTableNode.child(rowIndex);
+      const lastCellIndex = currentRow.childCount - 1;
+      const lastCell = currentRow.child(lastCellIndex);
+      const lastAttrs = lastCell.attrs as Record<string, unknown>;
+      const tableCellProperties = (lastAttrs.tableCellProperties ?? {}) as Record<string, unknown>;
+      const lastRowspan = Math.max(1, (lastAttrs.rowspan as number) || 1);
+      const hasVerticalMerge = tableCellProperties.vMerge != null;
+
+      if (lastRowspan > 1 || hasVerticalMerge) {
+        // Extending a vertically merged cell can overlap cells in lower rows.
+        let rowEndPos = tablePos + 1;
+        for (let i = 0; i <= rowIndex; i++) rowEndPos += currentTableNode.child(i).nodeSize;
+        const mappedRowEnd = rowEndPos - 1; // -1 to stay inside the row. No mapping needed — rowEndPos is already in post-delete doc space.
+        const newCell = schema.nodes.tableCell.createAndFill()!;
+        tr.insert(mappedRowEnd, newCell);
+      } else {
+        const lastColspan = Math.max(1, (lastAttrs.colspan as number) || 1);
+        const nextColspan = lastColspan + deletedColspan;
+
+        const nextTableCellProps = {
+          ...tableCellProperties,
+        };
+        if (nextColspan > 1) nextTableCellProps.gridSpan = nextColspan;
+        else delete nextTableCellProps.gridSpan;
+
+        const nextColwidth = Array.isArray(lastAttrs.colwidth) ? [...(lastAttrs.colwidth as number[])] : null;
+        if (nextColwidth) {
+          if (deletedColwidth) {
+            for (const width of deletedColwidth) {
+              if (nextColwidth.length >= nextColspan) break;
+              nextColwidth.push(typeof width === 'number' ? width : 0);
+            }
+          }
+          while (nextColwidth.length < nextColspan) nextColwidth.push(0);
+        }
+
+        let rowOffset = 0;
+        for (let i = 0; i < rowIndex; i++) rowOffset += currentTableNode.child(i).nodeSize;
+        let lastCellOffset = rowOffset + 1;
+        for (let i = 0; i < lastCellIndex; i++) lastCellOffset += currentRow.child(i).nodeSize;
+
+        tr.setNodeMarkup(tableStart + lastCellOffset, null, {
+          ...lastAttrs,
+          colspan: nextColspan,
+          colwidth: nextColwidth,
+          tableCellProperties: nextTableCellProps,
+        });
+      }
     } else {
       // shiftUp: insert a new empty cell at the same column in the last row.
       const lastRowIndex = map.height - 1;

tests/doc-api-stories/tests/tables/all-commands.ts

@@ -79,6 +79,7 @@ describe('document-api story: all table commands', () => {
   const insertCellTableBySession = new Map<string, string>();
   const insertCellInitialRowsBySession = new Map<string, number>();
   const deleteCellBySession = new Map<string, string>();
+  const deleteCellTableBySession = new Map<string, string>();
 
   function makeSessionId(prefix: string): string {
     return `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
@@ -690,20 +691,54 @@ describe('document-api story: all table commands', () => {
         }
 
         const cellsResult = unwrap<any>(await api.doc.tables.getCells({ sessionId, nodeId: tableNodeId }));
+        const firstRowBefore = Array.isArray(cellsResult?.cells)
+          ? cellsResult.cells.filter((cell: any) => cell?.rowIndex === 0)
+          : [];
+        if (firstRowBefore.length !== 3) {
+          throw new Error(`tables.deleteCell setup expected 3 cells in first row, received ${firstRowBefore.length}.`);
+        }
         const firstCellNodeId = cellsResult?.cells?.[0]?.nodeId;
         if (!firstCellNodeId) {
           throw new Error('tables.deleteCell setup failed: no table cell was returned from getCells.');
         }
 
         deleteCellBySession.set(sessionId, firstCellNodeId);
+        deleteCellTableBySession.set(sessionId, tableNodeId);
       },
       run: async (sessionId) => {
         const cellNodeId = deleteCellBySession.get(sessionId);
+        const tableNodeId = deleteCellTableBySession.get(sessionId);
         if (!cellNodeId) {
           throw new Error('tables.deleteCell setup failed: prepared cell nodeId was not found.');
         }
+        if (!tableNodeId) {
+          throw new Error('tables.deleteCell setup failed: prepared table nodeId was not found.');
+        }
         deleteCellBySession.delete(sessionId);
-        return unwrap<any>(await api.doc.tables.deleteCell({ sessionId, nodeId: cellNodeId, mode: 'shiftLeft' }));
+        deleteCellTableBySession.delete(sessionId);
+
+        const result = unwrap<any>(
+          await api.doc.tables.deleteCell({ sessionId, nodeId: cellNodeId, mode: 'shiftLeft' }),
+        );
+        assertMutationSuccess('tables.deleteCell', result);
+
+        const postCells = unwrap<any>(await api.doc.tables.getCells({ sessionId, nodeId: tableNodeId, rowIndex: 0 }));
+        const firstRowAfter = Array.isArray(postCells?.cells)
+          ? postCells.cells.filter((cell: any) => cell?.rowIndex === 0)
+          : [];
+        if (firstRowAfter.length !== 2) {
+          throw new Error(
+            `tables.deleteCell expected first-row cell count to be 2 after shiftLeft, received ${firstRowAfter.length}.`,
+          );
+        }
+        const firstRowColumns = firstRowAfter.map((cell: any) => Number(cell?.columnIndex)).sort((a, b) => a - b);
+        if (firstRowColumns.join(',') !== '0,1') {
+          throw new Error(
+            `tables.deleteCell expected first-row column indexes [0,1] after shiftLeft, received [${firstRowColumns.join(',')}].`,
+          );
+        }
+
+        return result;
       },
     },
     {