From 2b419fcb1a645937db0192521a1aedaca89a76d8 Mon Sep 17 00:00:00 2001 From: Ali Waseem Date: Thu, 26 Mar 2026 00:39:37 -0600 Subject: [PATCH] fix: revert changes when the value is the same as old (#44196) ## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Revert changes when the value is the same as the old value --------- Co-authored-by: Joshen Lim --- .../utils/queueConflictResolution.test.ts | 114 ++++++++++++++++++ .../grid/utils/queueConflictResolution.ts | 22 +++- .../features/queue-table-operations.spec.ts | 47 ++++++++ 3 files changed, 179 insertions(+), 4 deletions(-) diff --git a/apps/studio/components/grid/utils/queueConflictResolution.test.ts b/apps/studio/components/grid/utils/queueConflictResolution.test.ts index 7df3199c4b..77ddc7ca7e 100644 --- a/apps/studio/components/grid/utils/queueConflictResolution.test.ts +++ b/apps/studio/components/grid/utils/queueConflictResolution.test.ts @@ -491,6 +491,120 @@ describe('upsertOperation', () => { expect((updated.payload as any).newValue).toBe('final value') }) + test('should remove operation when value is reverted to original', () => { + const existingOp: QueuedOperation = { + id: 'edit_cell_content:1:name:id:1', + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + timestamp: Date.now() - 1000, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'name', + oldValue: 'original', + newValue: 'changed', + table: mockTable, + }, + } + + const otherOp: QueuedOperation = { + id: 'edit_cell_content:1:email:id:1', + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + timestamp: Date.now(), + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'email', + oldValue: 'old@test.com', + newValue: 'new@test.com', + table: mockTable, + }, + } + + const operations = [existingOp, otherOp] + const newOperation: NewEditCellContentOperation = { + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'name', + oldValue: 'changed', + newValue: 'original', + table: mockTable, + }, + } + + const result = upsertOperation(operations, newOperation) + + expect(result.operations).toHaveLength(1) + expect(result.operations[0]).toEqual(otherOp) + }) + + test('should remove operation when number oldValue matches string newValue', () => { + const existingOp: QueuedOperation = { + id: 'edit_cell_content:1:age:id:1', + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + timestamp: Date.now() - 1000, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'age', + oldValue: 42, + newValue: 'changed', + table: mockTable, + }, + } + + const operations = [existingOp] + const newOperation: NewEditCellContentOperation = { + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'age', + oldValue: 'changed', + newValue: '42', + table: mockTable, + }, + } + + const result = upsertOperation(operations, newOperation) + + expect(result.operations).toHaveLength(0) + }) + + test('should remove operation when string oldValue matches JSON object newValue', () => { + const existingOp: QueuedOperation = { + id: 'edit_cell_content:1:metadata:id:1', + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + timestamp: Date.now() - 1000, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'metadata', + oldValue: '{"key":"value","count":3}', + newValue: 'changed', + table: mockTable, + }, + } + + const operations = [existingOp] + const newOperation: NewEditCellContentOperation = { + type: QueuedOperationType.EDIT_CELL_CONTENT, + tableId: 1, + payload: { + rowIdentifiers: { id: 1 }, + columnName: 'metadata', + oldValue: 'changed', + newValue: { key: 'value', count: 3 }, + table: mockTable, + }, + } + + const result = upsertOperation(operations, newOperation) + + expect(result.operations).toHaveLength(0) + }) + test('should update existing DELETE_ROW operation', () => { const existingOp: QueuedOperation = { id: 'delete_row:1:id:1', diff --git a/apps/studio/components/grid/utils/queueConflictResolution.ts b/apps/studio/components/grid/utils/queueConflictResolution.ts index 4b96f38c40..a0d0a1a630 100644 --- a/apps/studio/components/grid/utils/queueConflictResolution.ts +++ b/apps/studio/components/grid/utils/queueConflictResolution.ts @@ -1,13 +1,16 @@ +import { isEqual } from 'lodash' + import { isPendingAddRow } from '../types' import { generateTableChangeKey, rowMatchesIdentifiers } from './queueOperationUtils' +import { tryParseJson } from '@/lib/helpers' import { - type NewDeleteRowOperation, - type NewEditCellContentOperation, + isDeleteRowOperation, + isEditCellContentOperation, NewQueuedOperation, QueuedOperation, QueuedOperationType, - isDeleteRowOperation, - isEditCellContentOperation, + type NewDeleteRowOperation, + type NewEditCellContentOperation, } from '@/state/table-editor-operation-queue.types' export type DeleteConflictResult = @@ -165,6 +168,17 @@ export function upsertOperation( existingOp.type === QueuedOperationType.EDIT_CELL_CONTENT ) { queuedOperation.payload.oldValue = existingOp.payload.oldValue + + const { oldValue, newValue } = queuedOperation.payload + // [Joshen] These comparisons by data type are because of how the table editor renders the values + if ( + (typeof oldValue === 'number' && Number(oldValue) === Number(newValue)) || + (typeof newValue === 'object' && isEqual(tryParseJson(oldValue), newValue)) || + oldValue === newValue + ) { + updatedOperations.splice(existingOpIndex, 1) + return { operations: updatedOperations } + } } updatedOperations[existingOpIndex] = queuedOperation diff --git a/e2e/studio/features/queue-table-operations.spec.ts b/e2e/studio/features/queue-table-operations.spec.ts index d100e23c75..bbc357bd4f 100644 --- a/e2e/studio/features/queue-table-operations.spec.ts +++ b/e2e/studio/features/queue-table-operations.spec.ts @@ -786,6 +786,53 @@ test.describe('Queue Table Operations', () => { await expect(page.getByRole('gridcell', { name: 'only in table 2' })).not.toBeVisible() }) + test('reverted cell edit clears the pending change', async ({ page, ref }) => { + const tableName = `${tableNamePrefix}_revert_edit` + const columnName = 'name' + + await using _ = await withSetupCleanup( + async () => { + await createTable(tableName, columnName, [{ name: 'original value' }]) + }, + async () => { + await dropTable(tableName) + } + ) + + await page.goto(toUrl(`/project/${ref}/editor?schema=public`)) + await enableQueueOperations(page) + await page.reload() + await waitForTableToLoad(page, ref) + + await page.getByRole('button', { name: `View ${tableName}`, exact: true }).click() + await page.waitForURL(/\/editor\/\d+\?schema=public$/) + + await expect(page.getByRole('gridcell', { name: 'original value' })).toBeVisible() + + // Edit the cell to a different value + const cell = page.getByRole('gridcell', { name: 'original value' }) + await cell.dblclick() + + const editor = page.getByRole('textbox', { name: /Editor content/ }) + await expect(editor).toBeVisible() + await editor.fill('changed value') + await page.keyboard.press('Enter') + + await expect(page.getByText('1 pending change')).toBeVisible() + + // Edit the cell back to the original value + const changedCell = page.getByRole('gridcell', { name: 'changed value' }) + await changedCell.dblclick() + + const editor2 = page.getByRole('textbox', { name: /Editor content/ }) + await expect(editor2).toBeVisible() + await editor2.fill('original value') + await page.keyboard.press('Enter') + + // The pending change should be cleared since the value was reverted + await expect(page.getByText('pending change')).not.toBeVisible() + }) + test('editing multiple columns via side panel queues all changes', async ({ page, ref }) => { const tableName = `${tableNamePrefix}_multi_col`