fix(tables): fold a rename into the write it accompanies

Closes the last partial-update window, properly rather than by pre-checking
around it.

A rename is metadata-only — `renameColumn`'s own comment says so: rows,
metadata, and workflow-group refs all key on the stable column id, so it is a
pure schema write. Nothing forced it to be its own transaction. Running it
separately is what created the window: whichever half committed first survived
a failure in the other, and no pre-flight guard can close a concurrent
collision because only the write itself sees authoritative state.

The four column writes now accept an optional `newName` and apply it through
one shared `applyPendingRename`, which validates the name shape and checks the
collision against the very schema snapshot that write is landing in. A combined
request rides the rename on whichever write runs last, so both halves commit
together or neither does — a concurrent claim on the name now aborts the whole
transaction instead of leaving the other change applied.

The routes also address every write by the column's stable id rather than its
name, so folding a rename into one write cannot break the next one's lookup.
A rename with nothing to ride on still runs standalone.

What remains partial is a type write followed by a failing constraints write —
two independently locked transactions, pre-existing, and untouched by this PR.

Author: waleedlatif1

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

@@ -113,6 +113,18 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
     expect(mockRenameColumn).not.toHaveBeenCalled()
   })
 
+  it('renames standalone when there is no other write to ride on', async () => {
+    mockRenameColumn.mockResolvedValue({ schema: { columns: [] } })
+
+    const response = await patch({ name: 'renamed' })
+
+    expect(response.status).toBe(200)
+    expect(mockRenameColumn).toHaveBeenCalledWith(
+      expect.objectContaining({ oldName: 'col_a', newName: 'renamed' }),
+      expect.any(String)
+    )
+  })
+
   it('leaves the column untouched when a typed write fails', async () => {
     mockCheckAccess.mockResolvedValue({
       ok: true,
@@ -177,7 +189,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
     expect(mockRenameColumn).not.toHaveBeenCalled()
   })
 
-  it('renames only after the typed write succeeds', async () => {
+  it('folds a rename into the typed write instead of running it separately', async () => {
     mockCheckAccess.mockResolvedValue({
       ok: true,
       table: {
@@ -190,15 +202,13 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
     const response = await patch({ name: 'renamed', currencyCode: 'eur' })
 
     expect(response.status).toBe(200)
-    expect(mockRenameColumn).toHaveBeenCalledTimes(1)
+    // One transaction, not two: the rename rides along with the currency write,
+    // so neither half can commit without the other.
+    expect(mockRenameColumn).not.toHaveBeenCalled()
     expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
-      // Targets the column's CURRENT name: the rename has not run yet. The
-      // contract upper-cases the code on the way in.
-      expect.objectContaining({ columnName: 'amount', currencyCode: 'EUR' }),
+      // Addressed by stable id; the contract upper-cases the code on the way in.
+      expect.objectContaining({ columnName: 'col_a', currencyCode: 'EUR', newName: 'renamed' }),
       expect.any(String)
     )
-    expect(mockUpdateColumnCurrency.mock.invocationCallOrder[0]).toBeLessThan(
-      mockRenameColumn.mock.invocationCallOrder[0]
-    )
   })
 })

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

@@ -19,7 +19,7 @@ import {
   updateColumnOptions,
   updateColumnType,
 } from '@/lib/table'
-import { columnMatchesRef } from '@/lib/table/column-keys'
+import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
 import { columnTypeById } from '@/lib/table/column-types'
 import { isSupportedCurrencyCode } from '@/lib/table/currency'
 import {
@@ -130,6 +130,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
     const currentColumn = table.schema.columns.find((c) =>
       columnMatchesRef(c, validated.columnName)
     )
+    // Address every write below by the stable id, not the name: a rename folded
+    // into one of them must not break the next one's lookup.
+    const columnRef = currentColumn ? getColumnId(currentColumn) : validated.columnName
+    // The constraints write below is a separate, unconditional step, so it is
+    // the last one whenever it runs — that is the write the rename rides on.
+    const constraintsChanging = updates.required !== undefined || updates.unique !== undefined
+    const renameWithTypedWrite =
+      updates.name && !constraintsChanging ? { newName: updates.name } : {}
     const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
 
     // Every write below is its own locked transaction, so one that is going to
@@ -185,14 +193,15 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnType(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           newType: updates.type as NonNullable<typeof updates.type>,
           ...(updates.options !== undefined ? { options: updates.options } : {}),
           ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
           ...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}),
           // Forwarded so the conversion validates against the constraint this
           // same request is about to set, not the column's current one.
           ...(updates.required !== undefined ? { required: updates.required } : {}),
+          ...renameWithTypedWrite,
         },
         requestId
       )
@@ -203,21 +212,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnCurrency(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           currencyCode: updates.currencyCode,
+          ...renameWithTypedWrite,
         },
         requestId
       )
     } else if (updates.options !== undefined || updates.multiple !== undefined) {
       updatedTable = await updateColumnOptions(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           options: updates.options ?? currentColumn?.options ?? [],
           ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
           // Forwarded so the removal guard validates against the constraint this
           // same request is about to set, not the column's current one.
           ...(updates.required !== undefined ? { required: updates.required } : {}),
+          ...renameWithTypedWrite,
         },
         requestId
       )
@@ -227,23 +238,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnConstraints(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           ...(updates.required !== undefined ? { required: updates.required } : {}),
           ...(updates.unique !== undefined ? { unique: updates.unique } : {}),
+          ...(updates.name ? { newName: updates.name } : {}),
         },
         requestId
       )
     }
 
-    // Rename LAST, on purpose. Each write above is its own locked transaction,
-    // and the pre-flight guards read a schema snapshot — so a concurrent change
-    // can still make one of them fail. Renaming first meant that failure
-    // returned an error with the rename already committed. Going last, a failed
-    // typed write leaves the column untouched, and a failed rename leaves the
-    // typed change applied under the old name, which is the recoverable half.
-    if (updates.name) {
+    // A rename rides along with the LAST write above, inside that write's
+    // transaction — a rename is metadata-only (rows key on the stable column
+    // id), so nothing forces it to be its own write, and folding it in is what
+    // stops a combined request from committing one half and then failing. Only
+    // a rename with nothing to ride on runs standalone.
+    if (updates.name && !updatedTable) {
       updatedTable = await renameColumn(
-        { tableId, oldName: validated.columnName, newName: updates.name },
+        { tableId, oldName: columnRef, newName: updates.name },
         requestId
       )
     }

apps/sim/app/api/v1/tables/[tableId]/columns/route.ts

@@ -18,7 +18,7 @@ import {
   updateColumnOptions,
   updateColumnType,
 } from '@/lib/table'
-import { columnMatchesRef } from '@/lib/table/column-keys'
+import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
 import { columnTypeById } from '@/lib/table/column-types'
 import { isSupportedCurrencyCode } from '@/lib/table/currency'
 import {
@@ -164,6 +164,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
     const currentColumn = table.schema.columns.find((c) =>
       columnMatchesRef(c, validated.columnName)
     )
+    // Address every write below by the stable id, not the name: a rename folded
+    // into one of them must not break the next one's lookup.
+    const columnRef = currentColumn ? getColumnId(currentColumn) : validated.columnName
+    // The constraints write below is a separate, unconditional step, so it is
+    // the last one whenever it runs — that is the write the rename rides on.
+    const constraintsChanging = updates.required !== undefined || updates.unique !== undefined
+    const renameWithTypedWrite =
+      updates.name && !constraintsChanging ? { newName: updates.name } : {}
     const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
 
     // Every write below is its own locked transaction, so one that is going to
@@ -219,14 +227,15 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnType(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           newType: updates.type as NonNullable<typeof updates.type>,
           ...(updates.options !== undefined ? { options: updates.options } : {}),
           ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
           ...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}),
           // Forwarded so the conversion validates against the constraint this
           // same request is about to set, not the column's current one.
           ...(updates.required !== undefined ? { required: updates.required } : {}),
+          ...renameWithTypedWrite,
         },
         requestId
       )
@@ -237,21 +246,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnCurrency(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           currencyCode: updates.currencyCode,
+          ...renameWithTypedWrite,
         },
         requestId
       )
     } else if (updates.options !== undefined || updates.multiple !== undefined) {
       updatedTable = await updateColumnOptions(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           options: updates.options ?? currentColumn?.options ?? [],
           ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
           // Forwarded so the removal guard validates against the constraint this
           // same request is about to set, not the column's current one.
           ...(updates.required !== undefined ? { required: updates.required } : {}),
+          ...renameWithTypedWrite,
         },
         requestId
       )
@@ -261,23 +272,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       updatedTable = await updateColumnConstraints(
         {
           tableId,
-          columnName: validated.columnName,
+          columnName: columnRef,
           ...(updates.required !== undefined ? { required: updates.required } : {}),
           ...(updates.unique !== undefined ? { unique: updates.unique } : {}),
+          ...(updates.name ? { newName: updates.name } : {}),
         },
         requestId
       )
     }
 
-    // Rename LAST, on purpose. Each write above is its own locked transaction,
-    // and the pre-flight guards read a schema snapshot — so a concurrent change
-    // can still make one of them fail. Renaming first meant that failure
-    // returned an error with the rename already committed. Going last, a failed
-    // typed write leaves the column untouched, and a failed rename leaves the
-    // typed change applied under the old name, which is the recoverable half.
-    if (updates.name) {
+    // A rename rides along with the LAST write above, inside that write's
+    // transaction — a rename is metadata-only (rows key on the stable column
+    // id), so nothing forces it to be its own write, and folding it in is what
+    // stops a combined request from committing one half and then failing. Only
+    // a rename with nothing to ride on runs standalone.
+    if (updates.name && !updatedTable) {
       updatedTable = await renameColumn(
-        { tableId, oldName: validated.columnName, newName: updates.name },
+        { tableId, oldName: columnRef, newName: updates.name },
         requestId
       )
     }

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

@@ -5,7 +5,11 @@
  * coverage and every case below was a shipped defect caught in review.
  */
 import { describe, expect, it } from 'vitest'
-import { isValueCompatibleWithType, selectValueForConversion } from '@/lib/table/columns/service'
+import {
+  applyPendingRename,
+  isValueCompatibleWithType,
+  selectValueForConversion,
+} from '@/lib/table/columns/service'
 import type { ColumnDefinition, SelectOption } from '@/lib/table/types'
 
 const OPTIONS: SelectOption[] = [
@@ -161,3 +165,32 @@ describe('blank cells during conversion', () => {
     expect(isValueCompatibleWithType('', 'select', OPTIONS, false, true)).toBe(false)
   })
 })
+
+describe('rename folded into another write', () => {
+  // A rename is metadata-only — rows key on the stable column id — so nothing
+  // forces it to be its own transaction. Folding it into whichever write a
+  // request already carries is what makes a combined PATCH all-or-nothing: the
+  // name collision is detected against the same schema snapshot the other
+  // change is being applied to, and both abort together.
+  it('rejects a collision against the schema the write is landing in', () => {
+    const columns: ColumnDefinition[] = [
+      { id: 'col_a', name: 'amount', type: 'currency' },
+      { id: 'col_b', name: 'taken', type: 'string' },
+    ]
+    expect(() => applyPendingRename(columns, 0, 'taken')).toThrow(/already exists/)
+    expect(() => applyPendingRename(columns, 0, 'TAKEN')).toThrow(/already exists/)
+  })
+
+  it('applies a valid rename and is a no-op without one', () => {
+    const columns: ColumnDefinition[] = [{ id: 'col_a', name: 'amount', type: 'currency' }]
+    expect(applyPendingRename(columns, 0, 'total').name).toBe('total')
+    expect(applyPendingRename(columns, 0, undefined)).toBe(columns[0])
+    expect(applyPendingRename(columns, 0, 'amount')).toBe(columns[0])
+  })
+
+  it('rejects a name the column-name rules forbid', () => {
+    const columns: ColumnDefinition[] = [{ id: 'col_a', name: 'amount', type: 'currency' }]
+    expect(() => applyPendingRename(columns, 0, '1bad')).toThrow(/must start with/)
+    expect(() => applyPendingRename(columns, 0, 'a'.repeat(200))).toThrow(/maximum length/)
+  })
+})

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

@@ -485,6 +485,43 @@ export async function deleteColumns(
   return def
 }
 
+/**
+ * Validates a pending rename against the schema it will land in, and returns
+ * the renamed column.
+ *
+ * Exists so a rename can be folded into whatever OTHER column write a request
+ * carries, inside that write's transaction. Each write is its own locked
+ * transaction, so a standalone rename alongside one of them means either order
+ * can commit and then fail — and since a rename is metadata-only (rows key on
+ * the stable column id), there is nothing forcing it to be its own write.
+ *
+ * Returns the column unchanged when there is no rename to apply. Exported so
+ * the collision and name-shape rules are testable without a transaction.
+ */
+export function applyPendingRename(
+  columns: ColumnDefinition[],
+  columnIndex: number,
+  newName: string | undefined
+): ColumnDefinition {
+  const column = columns[columnIndex]
+  if (newName === undefined || newName === column.name) return column
+
+  if (!NAME_PATTERN.test(newName)) {
+    throw new Error(
+      `Invalid column name "${newName}". Column names must start with a letter or underscore, followed by alphanumeric characters or underscores.`
+    )
+  }
+  if (newName.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) {
+    throw new Error(
+      `Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)`
+    )
+  }
+  if (columns.some((c, i) => i !== columnIndex && c.name.toLowerCase() === newName.toLowerCase())) {
+    throw new Error(`Column "${newName}" already exists`)
+  }
+  return { ...column, name: newName }
+}
+
 /**
  * The column definition a retype produces: prior per-type metadata dropped,
  * then only what the TARGET type declares it owns carried forward, then that
@@ -694,7 +731,10 @@ export async function updateColumnType(
       )
     }
 
-    const updatedColumns = schema.columns.map((c, i) => (i === columnIndex ? convertedColumn : c))
+    const renamedColumns = schema.columns.map((c, i) => (i === columnIndex ? convertedColumn : c))
+    const updatedColumns = renamedColumns.map((c, i) =>
+      i === columnIndex ? applyPendingRename(renamedColumns, columnIndex, data.newName) : c
+    )
 
     const columnValidation = validateColumnDefinition(updatedColumns[columnIndex])
     if (!columnValidation.valid) {
@@ -798,7 +838,7 @@ export async function updateColumnConstraints(
       }
     }
 
-    const updatedColumns = schema.columns.map((c, i) =>
+    const withConstraints = schema.columns.map((c, i) =>
       i === columnIndex
         ? {
             ...c,
@@ -807,6 +847,9 @@ export async function updateColumnConstraints(
           }
         : c
     )
+    const updatedColumns = withConstraints.map((c, i) =>
+      i === columnIndex ? applyPendingRename(withConstraints, columnIndex, data.newName) : c
+    )
     const updatedSchema: TableSchema = { ...schema, columns: updatedColumns }
     const now = new Date()
 
@@ -858,7 +901,10 @@ export async function updateColumnOptions(
       throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`)
     }
 
-    const updatedColumns = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
+    const withOptions = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
+    const updatedColumns = withOptions.map((c, i) =>
+      i === columnIndex ? applyPendingRename(withOptions, columnIndex, data.newName) : c
+    )
     const updatedSchema: TableSchema = { ...schema, columns: updatedColumns }
     const now = new Date()
 
@@ -1024,7 +1070,10 @@ export async function updateColumnCurrency(
       return table
     }
 
-    const updatedColumns = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
+    const withCurrency = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
+    const updatedColumns = withCurrency.map((c, i) =>
+      i === columnIndex ? applyPendingRename(withCurrency, columnIndex, data.newName) : c
+    )
     const updatedSchema: TableSchema = { ...schema, columns: updatedColumns }
     const now = new Date()