Skip to content

Commit 9ff07f1

Browse files
committed
fix(review): serialize inline column renames
1 parent 999bfb4 commit 9ff07f1

3 files changed

Lines changed: 170 additions & 20 deletions

File tree

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act } from 'react'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
7+
import {
8+
persistColumnRename,
9+
tryStartColumnRename,
10+
} from '@/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/column-rename'
11+
import { useInlineRename } from '@/hooks/use-inline-rename'
12+
13+
interface Deferred {
14+
promise: Promise<void>
15+
resolve: () => void
16+
}
17+
18+
function createDeferred(): Deferred {
19+
let resolve = () => {}
20+
const promise = new Promise<void>((settle) => {
21+
resolve = settle
22+
})
23+
return { promise, resolve }
24+
}
25+
26+
describe('column rename persistence', () => {
27+
it('does not register undo history when persistence rejects', async () => {
28+
const error = new Error('rename rejected')
29+
const pushUndo = vi.fn()
30+
const onRenamed = vi.fn()
31+
32+
await expect(
33+
persistColumnRename({
34+
columnId: 'column-1',
35+
oldName: 'Original',
36+
newName: 'Updated',
37+
persist: () => Promise.reject(error),
38+
pushUndo,
39+
onRenamed,
40+
})
41+
).rejects.toBe(error)
42+
43+
expect(pushUndo).not.toHaveBeenCalled()
44+
expect(onRenamed).not.toHaveBeenCalled()
45+
})
46+
})
47+
48+
describe('column rename sessions', () => {
49+
let container: HTMLDivElement
50+
let root: Root
51+
let rename: ReturnType<typeof useInlineRename>
52+
53+
beforeEach(() => {
54+
globalThis.IS_REACT_ACT_ENVIRONMENT = true
55+
container = document.createElement('div')
56+
document.body.appendChild(container)
57+
root = createRoot(container)
58+
})
59+
60+
afterEach(() => {
61+
act(() => root.unmount())
62+
container.remove()
63+
})
64+
65+
it('refuses a second session until the pending rename settles', async () => {
66+
const deferred = createDeferred()
67+
68+
function Harness() {
69+
rename = useInlineRename({ onSave: () => deferred.promise })
70+
return null
71+
}
72+
73+
act(() => root.render(<Harness />))
74+
act(() => {
75+
expect(tryStartColumnRename(rename, 'column-1', 'First')).toBe(true)
76+
})
77+
act(() => rename.setEditValue('Renamed first'))
78+
79+
let pendingRename: Promise<void>
80+
act(() => {
81+
pendingRename = rename.submitRename()
82+
})
83+
84+
expect(rename.isSaving).toBe(true)
85+
act(() => {
86+
expect(tryStartColumnRename(rename, 'column-2', 'Second')).toBe(false)
87+
})
88+
expect(rename.editingId).toBe('column-1')
89+
90+
await act(async () => {
91+
deferred.resolve()
92+
await pendingRename
93+
})
94+
95+
expect(rename.isSaving).toBe(false)
96+
act(() => {
97+
expect(tryStartColumnRename(rename, 'column-2', 'Second')).toBe(true)
98+
})
99+
expect(rename.editingId).toBe('column-2')
100+
})
101+
})
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import type { TableUndoAction } from '@/stores/table/types'
2+
3+
type RenameColumnUndoAction = Extract<TableUndoAction, { type: 'rename-column' }>
4+
5+
interface PersistColumnRenameOptions {
6+
columnId: string
7+
oldName: string
8+
newName: string
9+
persist: () => Promise<unknown>
10+
pushUndo: (action: RenameColumnUndoAction) => void
11+
onRenamed: () => void
12+
}
13+
14+
export async function persistColumnRename({
15+
columnId,
16+
oldName,
17+
newName,
18+
persist,
19+
pushUndo,
20+
onRenamed,
21+
}: PersistColumnRenameOptions): Promise<void> {
22+
await persist()
23+
pushUndo({ type: 'rename-column', oldName, newName, columnId })
24+
onRenamed()
25+
}
26+
27+
interface InlineRenameSession {
28+
isSaving: boolean
29+
startRename: (id: string, currentName: string) => void
30+
}
31+
32+
export function tryStartColumnRename(
33+
session: InlineRenameSession,
34+
columnId: string,
35+
currentName: string
36+
): boolean {
37+
if (session.isSaving) return false
38+
session.startRename(columnId, currentName)
39+
return true
40+
}

‎apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ import { cellValueFilterConditions } from '@/lib/table/query-builder/cell-filter
3737
import { SEARCH_DEBOUNCE_MS } from '@/lib/url-state'
3838
import { FindBar } from '@/app/workspace/[workspaceId]/components'
3939
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
40+
import {
41+
persistColumnRename,
42+
tryStartColumnRename,
43+
} from '@/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/column-rename'
4044
import {
4145
REFERENCE_ROW_PREVIEW_HEIGHT,
4246
ReferenceRowPreview,
@@ -1590,22 +1594,28 @@ export function TableGrid({
15901594
const columnRename = useInlineRename({
15911595
// `columnName` is the column id; record the prior display name + id so undo
15921596
// restores the label (not the id) and targets the right column.
1593-
onSave: (columnName, newName) => {
1597+
onSave: async (columnName, newName) => {
15941598
const oldName = columnsRef.current.find((c) => c.key === columnName)?.name ?? columnName
1595-
pushUndoRef.current({ type: 'rename-column', oldName, newName, columnId: columnName })
1596-
handleColumnRename(columnName, newName)
1597-
return updateColumnMutation
1598-
.mutateAsync({ columnName, updates: { name: newName } })
1599-
.catch((error: unknown) => {
1600-
if (isValidationError(error)) {
1601-
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1602-
}
1603-
setRenameErrorColumnId(columnName)
1604-
throw error
1599+
try {
1600+
await persistColumnRename({
1601+
columnId: columnName,
1602+
oldName,
1603+
newName,
1604+
persist: () =>
1605+
updateColumnMutation.mutateAsync({ columnName, updates: { name: newName } }),
1606+
pushUndo: pushUndoRef.current,
1607+
onRenamed: () => handleColumnRename(columnName, newName),
16051608
})
1609+
} catch (error) {
1610+
if (isValidationError(error)) {
1611+
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1612+
}
1613+
setRenameErrorColumnId(columnName)
1614+
throw error
1615+
}
16061616
},
16071617
})
1608-
const columnRenameRef = useRef(columnRename)
1618+
const columnRenameRef = useRef<ReturnType<typeof useInlineRename>>(columnRename)
16091619
columnRenameRef.current = columnRename
16101620

16111621
const handleRenameValueChange = useCallback((value: string) => {
@@ -4171,14 +4181,13 @@ export function TableGrid({
41714181
[onOpenColumnConfig, onOpenWorkflowConfig, workflowGroupById]
41724182
)
41734183

4174-
const handleRenameColumn = useCallback(
4175-
(columnName: string) => {
4176-
setRenameErrorColumnId(null)
4177-
const column = columnsRef.current.find((candidate) => candidate.key === columnName)
4178-
columnRename.startRename(columnName, column?.name ?? columnName)
4179-
},
4180-
[columnRename.startRename]
4181-
)
4184+
const handleRenameColumn = useCallback((columnName: string) => {
4185+
const column = columnsRef.current.find((candidate) => candidate.key === columnName)
4186+
if (!tryStartColumnRename(columnRenameRef.current, columnName, column?.name ?? columnName)) {
4187+
return
4188+
}
4189+
setRenameErrorColumnId(null)
4190+
}, [])
41824191

41834192
const handleConfigureWorkflowGroup = useCallback(
41844193
(groupId: string) => {

0 commit comments

Comments
 (0)