Skip to content

Commit 1da8220

Browse files
committed
fix(review): serialize inline column renames
1 parent d4aac03 commit 1da8220

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
@@ -32,6 +32,10 @@ import { cellValueFilterConditions } from '@/lib/table/query-builder/cell-filter
3232
import { SEARCH_DEBOUNCE_MS } from '@/lib/url-state'
3333
import { FindBar } from '@/app/workspace/[workspaceId]/components'
3434
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
35+
import {
36+
persistColumnRename,
37+
tryStartColumnRename,
38+
} from '@/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/column-rename'
3539
import { getTimezoneEditBlockedMessage } from '@/app/workspace/[workspaceId]/tables/[tableId]/components/timezone-editing'
3640
import type { RemoteTableSelection } from '@/app/workspace/[workspaceId]/tables/[tableId]/hooks/use-table-room'
3741
import type { BlockedTableAction } from '@/app/workspace/[workspaceId]/tables/[tableId]/lock-copy'
@@ -1509,22 +1513,28 @@ export function TableGrid({
15091513
const columnRename = useInlineRename({
15101514
// `columnName` is the column id; record the prior display name + id so undo
15111515
// restores the label (not the id) and targets the right column.
1512-
onSave: (columnName, newName) => {
1516+
onSave: async (columnName, newName) => {
15131517
const oldName = columnsRef.current.find((c) => c.key === columnName)?.name ?? columnName
1514-
pushUndoRef.current({ type: 'rename-column', oldName, newName, columnId: columnName })
1515-
handleColumnRename(columnName, newName)
1516-
return updateColumnMutation
1517-
.mutateAsync({ columnName, updates: { name: newName } })
1518-
.catch((error: unknown) => {
1519-
if (isValidationError(error)) {
1520-
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1521-
}
1522-
setRenameErrorColumnId(columnName)
1523-
throw error
1518+
try {
1519+
await persistColumnRename({
1520+
columnId: columnName,
1521+
oldName,
1522+
newName,
1523+
persist: () =>
1524+
updateColumnMutation.mutateAsync({ columnName, updates: { name: newName } }),
1525+
pushUndo: pushUndoRef.current,
1526+
onRenamed: () => handleColumnRename(columnName, newName),
15241527
})
1528+
} catch (error) {
1529+
if (isValidationError(error)) {
1530+
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1531+
}
1532+
setRenameErrorColumnId(columnName)
1533+
throw error
1534+
}
15251535
},
15261536
})
1527-
const columnRenameRef = useRef(columnRename)
1537+
const columnRenameRef = useRef<ReturnType<typeof useInlineRename>>(columnRename)
15281538
columnRenameRef.current = columnRename
15291539

15301540
const handleRenameValueChange = useCallback((value: string) => {
@@ -4033,14 +4043,13 @@ export function TableGrid({
40334043
[onOpenColumnConfig, onOpenWorkflowConfig, workflowGroupById]
40344044
)
40354045

4036-
const handleRenameColumn = useCallback(
4037-
(columnName: string) => {
4038-
setRenameErrorColumnId(null)
4039-
const column = columnsRef.current.find((candidate) => candidate.key === columnName)
4040-
columnRename.startRename(columnName, column?.name ?? columnName)
4041-
},
4042-
[columnRename.startRename]
4043-
)
4046+
const handleRenameColumn = useCallback((columnName: string) => {
4047+
const column = columnsRef.current.find((candidate) => candidate.key === columnName)
4048+
if (!tryStartColumnRename(columnRenameRef.current, columnName, column?.name ?? columnName)) {
4049+
return
4050+
}
4051+
setRenameErrorColumnId(null)
4052+
}, [])
40444053

40454054
const handleConfigureWorkflowGroup = useCallback(
40464055
(groupId: string) => {

0 commit comments

Comments
 (0)