Skip to content

Commit 542f966

Browse files
committed
fix(tables): don't drop a rename when the write it rides on no-ops
Found by Cursor Bugbot — a bug I introduced folding the rename in. `updateColumnCurrency` returns early when the code is unchanged, and that return sat ahead of the rename, so PATCH {name, currencyCode} with the column's current code answered 200 with the rename silently discarded. Both early returns now treat a pending rename as work: the currency path only no-ops when the code is unchanged AND no rename is riding along, and the retype path applies a rename-only write when the type is unchanged. `applyPendingRename` signals "nothing to do" by returning the same reference, which is what lets both detect it cleanly. Also extracts `persistColumns` — five sites were repeating the same schema-write-and-return.
1 parent 1de0f14 commit 542f966

3 files changed

Lines changed: 59 additions & 2 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,29 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
113113
expect(mockRenameColumn).not.toHaveBeenCalled()
114114
})
115115

116+
it('still applies a rename when the currency it rides on is unchanged', async () => {
117+
// `updateColumnCurrency` no-ops on an unchanged code. The rename folded into
118+
// the same request must not be dropped with it.
119+
mockCheckAccess.mockResolvedValue({
120+
ok: true,
121+
table: {
122+
workspaceId: WORKSPACE_ID,
123+
schema: {
124+
columns: [{ id: 'col_a', name: 'amount', type: 'currency', currencyCode: 'USD' }],
125+
},
126+
},
127+
})
128+
mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } })
129+
130+
const response = await patch({ name: 'renamed', currencyCode: 'USD' })
131+
132+
expect(response.status).toBe(200)
133+
expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
134+
expect.objectContaining({ currencyCode: 'USD', newName: 'renamed' }),
135+
expect.any(String)
136+
)
137+
})
138+
116139
it('renames standalone when there is no other write to ride on', async () => {
117140
mockRenameColumn.mockResolvedValue({ schema: { columns: [] } })
118141

apps/sim/lib/table/__tests__/column-conversion.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,15 @@ describe('rename folded into another write', () => {
182182
expect(() => applyPendingRename(columns, 0, 'TAKEN')).toThrow(/already exists/)
183183
})
184184

185+
it('signals a no-op by identity, which is how callers detect nothing to write', () => {
186+
// The early returns in `updateColumnType` / `updateColumnCurrency` rely on
187+
// this: same reference means there is genuinely nothing to persist.
188+
const columns: ColumnDefinition[] = [{ id: 'col_a', name: 'amount', type: 'currency' }]
189+
expect(applyPendingRename(columns, 0, undefined)).toBe(columns[0])
190+
expect(applyPendingRename(columns, 0, 'amount')).toBe(columns[0])
191+
expect(applyPendingRename(columns, 0, 'renamed')).not.toBe(columns[0])
192+
})
193+
185194
it('applies a valid rename and is a no-op without one', () => {
186195
const columns: ColumnDefinition[] = [{ id: 'col_a', name: 'amount', type: 'currency' }]
187196
expect(applyPendingRename(columns, 0, 'total').name).toBe('total')

apps/sim/lib/table/columns/service.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,21 @@ export async function deleteColumns(
485485
return def
486486
}
487487

488+
/** Persists a column list as the table's schema and returns the updated definition. */
489+
async function persistColumns(
490+
trx: DbTransaction,
491+
table: TableDefinition,
492+
columns: ColumnDefinition[]
493+
): Promise<TableDefinition> {
494+
const updatedSchema: TableSchema = { ...table.schema, columns }
495+
const now = new Date()
496+
await trx
497+
.update(userTableDefinitions)
498+
.set({ schema: updatedSchema, updatedAt: now })
499+
.where(eq(userTableDefinitions.id, table.id))
500+
return { ...table, schema: updatedSchema, updatedAt: now }
501+
}
502+
488503
/**
489504
* Whether any two rows share a stored value in this column.
490505
*
@@ -627,7 +642,15 @@ export async function updateColumnType(
627642

628643
const column = schema.columns[columnIndex]
629644
if (column.type === data.newType) {
630-
return table
645+
// The type is unchanged, but a rename folded into this same request still
646+
// has to land — returning here unconditionally would drop it silently.
647+
const renamed = applyPendingRename(schema.columns, columnIndex, data.newName)
648+
if (renamed === column) return table
649+
return persistColumns(
650+
trx,
651+
table,
652+
schema.columns.map((c, i) => (i === columnIndex ? renamed : c))
653+
)
631654
}
632655
const columnKey = getColumnId(column)
633656

@@ -1097,7 +1120,9 @@ export async function updateColumnCurrency(
10971120
throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`)
10981121
}
10991122

1100-
if (updatedColumn.currencyCode === column.currencyCode) {
1123+
// Only a no-op when the currency is unchanged AND no rename is riding along.
1124+
const renamePending = data.newName !== undefined && data.newName !== column.name
1125+
if (updatedColumn.currencyCode === column.currencyCode && !renamePending) {
11011126
return table
11021127
}
11031128

0 commit comments

Comments
 (0)