Skip to content

Commit 7263fa7

Browse files
committed
fix(review): serialize inline column renames
1 parent c406724 commit 7263fa7

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'
@@ -1511,22 +1515,28 @@ export function TableGrid({
15111515
const columnRename = useInlineRename({
15121516
// `columnName` is the column id; record the prior display name + id so undo
15131517
// restores the label (not the id) and targets the right column.
1514-
onSave: (columnName, newName) => {
1518+
onSave: async (columnName, newName) => {
15151519
const oldName = columnsRef.current.find((c) => c.key === columnName)?.name ?? columnName
1516-
pushUndoRef.current({ type: 'rename-column', oldName, newName, columnId: columnName })
1517-
handleColumnRename(columnName, newName)
1518-
return updateColumnMutation
1519-
.mutateAsync({ columnName, updates: { name: newName } })
1520-
.catch((error: unknown) => {
1521-
if (isValidationError(error)) {
1522-
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1523-
}
1524-
setRenameErrorColumnId(columnName)
1525-
throw error
1520+
try {
1521+
await persistColumnRename({
1522+
columnId: columnName,
1523+
oldName,
1524+
newName,
1525+
persist: () =>
1526+
updateColumnMutation.mutateAsync({ columnName, updates: { name: newName } }),
1527+
pushUndo: pushUndoRef.current,
1528+
onRenamed: () => handleColumnRename(columnName, newName),
15261529
})
1530+
} catch (error) {
1531+
if (isValidationError(error)) {
1532+
toast.error(extractValidationIssues(error)[0]?.message ?? getErrorMessage(error))
1533+
}
1534+
setRenameErrorColumnId(columnName)
1535+
throw error
1536+
}
15271537
},
15281538
})
1529-
const columnRenameRef = useRef(columnRename)
1539+
const columnRenameRef = useRef<ReturnType<typeof useInlineRename>>(columnRename)
15301540
columnRenameRef.current = columnRename
15311541

15321542
const handleRenameValueChange = useCallback((value: string) => {
@@ -4035,14 +4045,13 @@ export function TableGrid({
40354045
[onOpenColumnConfig, onOpenWorkflowConfig, workflowGroupById]
40364046
)
40374047

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

40474056
const handleConfigureWorkflowGroup = useCallback(
40484057
(groupId: string) => {

0 commit comments

Comments
 (0)