Refactor updateCell to use updateCellBatch in SQLite engine

Refactored `updateCell` in `src/core/sqlite-db.ts` to delegate to `updateCellBatch`, eliminating duplicated logic for cell updates and JSON patching.
Also updated `undoModification` and `redoModification` to use `updateCellBatch` for batch operations, removing explicit transaction loops and preventing nested transaction errors.
Enhanced `updateCellBatch` with row ID validation and improved JSON object detection.
Added unit tests in `tests/unit/cell_update_refactor.test.ts`.

Co-authored-by: zknpr <96851588+zknpr@users.noreply.github.com>

Author: google-labs-jules[bot]

src/core/sqlite-db.ts

@@ -122,16 +122,12 @@ class WasmDatabaseEngine implements DatabaseOperations {
         case 'cell_update':
             if (affectedCells) {
                 // Batch undo
-                await this.executeQuery('BEGIN TRANSACTION');
-                try {
-                    for (const cell of affectedCells) {
-                        await this.updateCell(targetTable, cell.rowId, cell.columnName, cell.priorValue ?? null);
-                    }
-                    await this.executeQuery('COMMIT');
-                } catch (e) {
-                    await this.executeQuery('ROLLBACK');
-                    throw e;
-                }
+                const updates: CellUpdate[] = affectedCells.map(cell => ({
+                    rowId: cell.rowId,
+                    column: cell.columnName,
+                    value: cell.priorValue ?? null
+                }));
+                await this.updateCellBatch(targetTable, updates);
             } else if (targetRowId !== undefined && targetColumn) {
                 // Single cell undo
                 await this.updateCell(targetTable, targetRowId, targetColumn, priorValue ?? null);
@@ -214,16 +210,12 @@ class WasmDatabaseEngine implements DatabaseOperations {
         case 'cell_update':
             if (affectedCells) {
                 // Batch redo
-                await this.executeQuery('BEGIN TRANSACTION');
-                try {
-                    for (const cell of affectedCells) {
-                        await this.updateCell(targetTable, cell.rowId, cell.columnName, cell.newValue ?? null);
-                    }
-                    await this.executeQuery('COMMIT');
-                } catch (e) {
-                    await this.executeQuery('ROLLBACK');
-                    throw e;
-                }
+                const updates: CellUpdate[] = affectedCells.map(cell => ({
+                    rowId: cell.rowId,
+                    column: cell.columnName,
+                    value: cell.newValue ?? null
+                }));
+                await this.updateCellBatch(targetTable, updates);
             } else if (targetRowId !== undefined && targetColumn) {
                 await this.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null);
             }
@@ -297,43 +289,13 @@ class WasmDatabaseEngine implements DatabaseOperations {
    * Update a single cell value.
    */
   async updateCell(table: string, rowId: RecordId, column: string, value: CellValue, patch?: string): Promise<void> {
-    // Validate rowId is a number
-    const rowIdNum = Number(rowId);
-    if (!Number.isFinite(rowIdNum)) {
-      throw new Error(`Invalid rowid: ${rowId}`);
-    }
-
-    let sql: string;
-    let params: CellValue[];
-
-    if (patch) {
-        // Fallback to JS implementation of json_patch
-        // Fetch current value
-        const currentResult = await this.executeQuery(`SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(table)} WHERE rowid = ?`, [rowIdNum]);
-        let currentValue = currentResult[0]?.rows[0]?.[0];
-
-        // Parse current JSON
-        let currentObj = {};
-        if (typeof currentValue === 'string') {
-            try { currentObj = JSON.parse(currentValue); } catch {}
-        } else if (typeof currentValue === 'object' && currentValue !== null && !(currentValue instanceof Uint8Array)) {
-             // Already an object? (unlikely from SQLite unless using some extension, usually string)
-             currentObj = currentValue;
-        }
-
-        // Apply patch
-        const patchObj = typeof patch === 'string' ? JSON.parse(patch) : patch;
-        const newValueObj = applyMergePatch(currentObj, patchObj);
-        const newValueStr = JSON.stringify(newValueObj);
-
-        sql = `UPDATE ${escapeIdentifier(table)} SET ${escapeIdentifier(column)} = ? WHERE rowid = ?`;
-        params = [newValueStr, rowIdNum];
-    } else {
-        sql = `UPDATE ${escapeIdentifier(table)} SET ${escapeIdentifier(column)} = ? WHERE rowid = ?`;
-        params = [value, rowIdNum];
-    }
-
-    await this.executeQuery(sql, params);
+    const update: CellUpdate = {
+        rowId,
+        column,
+        value: patch !== undefined ? patch : value,
+        operation: patch !== undefined ? 'json_patch' : 'set'
+    };
+    return this.updateCellBatch(table, [update]);
   }
 
   /**
@@ -460,6 +422,10 @@ class WasmDatabaseEngine implements DatabaseOperations {
               for (const update of columnUpdates) {
                   const rowIdNum = Number(update.rowId);
 
+                  if (!Number.isFinite(rowIdNum)) {
+                    throw new Error(`Invalid rowid: ${update.rowId}`);
+                  }
+
                   if (op === 'json_patch') {
                      // Read using prepared statement
                      // selectStmt.get([rowIdNum]) returns [val] or undefined
@@ -479,6 +445,9 @@ class WasmDatabaseEngine implements DatabaseOperations {
                      let currentObj = {};
                      if (typeof currentValue === 'string') {
                          try { currentObj = JSON.parse(currentValue); } catch {}
+                     } else if (typeof currentValue === 'object' && currentValue !== null && !(currentValue instanceof Uint8Array)) {
+                         // Already an object? (unlikely from SQLite unless using some extension, usually string)
+                         currentObj = currentValue;
                      }
 
                      const patchObj = typeof update.value === 'string' ? JSON.parse(update.value as string) : update.value;

tests/unit/cell_update_refactor.test.ts

@@ -0,0 +1,137 @@
+
+import { describe, it, before, after } from 'node:test';
+import assert from 'node:assert';
+import { createDatabaseEngine } from '../../src/core/sqlite-db';
+import { CellUpdate, ModificationEntry } from '../../src/core/types';
+
+describe('SQLite Engine Cell Update Refactoring', () => {
+    let engine: any;
+
+    before(async () => {
+        // Initialize with empty DB
+        const result = await createDatabaseEngine({
+            content: null,
+            maxSize: 0,
+            readOnlyMode: false
+        });
+        engine = result.operations;
+
+        // Setup table
+        await engine.executeQuery("CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT, data TEXT)");
+        await engine.insertRow('users', { id: 1, name: 'Alice', data: '{"score": 10}' });
+        await engine.insertRow('users', { id: 2, name: 'Bob', data: '{"score": 20}' });
+    });
+
+    it('should update single cell', async () => {
+        await engine.updateCell('users', 1, 'name', 'AliceUpdated');
+        const result = await engine.executeQuery("SELECT name FROM users WHERE id = 1");
+        assert.strictEqual(result[0].rows[0][0], 'AliceUpdated');
+    });
+
+    it('should update single cell with JSON patch', async () => {
+        // Apply patch to data column
+        await engine.updateCell('users', 1, 'data', null, '{"score": 15}');
+        const result = await engine.executeQuery("SELECT data FROM users WHERE id = 1");
+        const data = JSON.parse(result[0].rows[0][0] as string);
+        assert.strictEqual(data.score, 15);
+    });
+
+    it('should update multiple cells in batch', async () => {
+        const updates: CellUpdate[] = [
+            { rowId: 1, column: 'name', value: 'AliceBatch' },
+            { rowId: 2, column: 'name', value: 'BobBatch' }
+        ];
+        await engine.updateCellBatch('users', updates);
+
+        const result = await engine.executeQuery("SELECT name FROM users ORDER BY id");
+        assert.strictEqual(result[0].rows[0][0], 'AliceBatch');
+        assert.strictEqual(result[0].rows[1][0], 'BobBatch');
+    });
+
+    it('should update multiple cells with JSON patch in batch', async () => {
+        const updates: CellUpdate[] = [
+            { rowId: 1, column: 'data', value: '{"score": 25}', operation: 'json_patch' },
+            { rowId: 2, column: 'data', value: '{"score": 30}', operation: 'json_patch' }
+        ];
+        await engine.updateCellBatch('users', updates);
+
+        const result = await engine.executeQuery("SELECT data FROM users ORDER BY id");
+        const data1 = JSON.parse(result[0].rows[0][0] as string);
+        const data2 = JSON.parse(result[0].rows[1][0] as string);
+        assert.strictEqual(data1.score, 25);
+        assert.strictEqual(data2.score, 30);
+    });
+
+    it('should undo batch cell update', async () => {
+        // Setup initial state
+        await engine.updateCell('users', 1, 'name', 'AlicePreUndo');
+        await engine.updateCell('users', 2, 'name', 'BobPreUndo');
+
+        // Apply batch update (to be undone)
+        const updates: CellUpdate[] = [
+            { rowId: 1, column: 'name', value: 'AliceNew' },
+            { rowId: 2, column: 'name', value: 'BobNew' }
+        ];
+        await engine.updateCellBatch('users', updates);
+
+        // Verify update
+        let result = await engine.executeQuery("SELECT name FROM users ORDER BY id");
+        assert.strictEqual(result[0].rows[0][0], 'AliceNew');
+        assert.strictEqual(result[0].rows[1][0], 'BobNew');
+
+        // Undo
+        const mod: ModificationEntry = {
+            modificationType: 'cell_update',
+            targetTable: 'users',
+            description: 'Batch update',
+            affectedCells: [
+                { rowId: 1, columnName: 'name', priorValue: 'AlicePreUndo', newValue: 'AliceNew' },
+                { rowId: 2, columnName: 'name', priorValue: 'BobPreUndo', newValue: 'BobNew' }
+            ]
+        };
+        await engine.undoModification(mod);
+
+        // Verify undo
+        result = await engine.executeQuery("SELECT name FROM users ORDER BY id");
+        assert.strictEqual(result[0].rows[0][0], 'AlicePreUndo');
+        assert.strictEqual(result[0].rows[1][0], 'BobPreUndo');
+    });
+
+    it('should fail transaction on error', async () => {
+        const updates: CellUpdate[] = [
+             { rowId: 1, column: 'name', value: 'AliceTrans' },
+             // Invalid column to force error
+             { rowId: 2, column: 'non_existent_column', value: 'BobTrans' }
+        ];
+
+        try {
+            await engine.updateCellBatch('users', updates);
+            assert.fail('Should have thrown');
+        } catch (e) {
+            // Check that first update was rolled back
+            const result = await engine.executeQuery("SELECT name FROM users WHERE id = 1");
+            assert.strictEqual(result[0].rows[0][0], 'AlicePreUndo'); // Value from previous test
+        }
+    });
+
+    it('should validate rowId in updateCell', async () => {
+        try {
+            await engine.updateCell('users', 'invalid', 'name', 'test');
+            assert.fail('Should have thrown invalid rowid');
+        } catch (e: any) {
+            assert.match(e.message, /Invalid rowid/);
+        }
+    });
+
+    it('should validate rowId in updateCellBatch', async () => {
+        const updates: CellUpdate[] = [
+            { rowId: 'invalid', column: 'name', value: 'test' }
+        ];
+        try {
+            await engine.updateCellBatch('users', updates);
+            assert.fail('Should have thrown invalid rowid');
+        } catch (e: any) {
+            assert.match(e.message, /Invalid rowid/);
+        }
+    });
+});