Skip to content

Commit 0396f3f

Browse files
committed
fix(tables): audit fixes — column-copy drain, polling scope, merge identity
- Fix polling tick: move Promise.all inside else-branch so dirty[] stays in scope; keep hasDirty=true during active mutations so the short interval fires while chunked batch-updates are in flight - Add isColumnSelectionRef branch to handleCopy (mirrors handleCut fix): column-header Cmd+C now drains all pages before building clipboard content - Replace String(updatedAt) comparison in mergePagePreservingIdentity with Date.getTime() equality — handles ISO vs +00:00 timezone variants - Remove redundant batchUpdates.length > 0 guards at chunkBatchUpdates callsites (empty-array case is handled inside the function) - Export _mergePagePreservingIdentity for unit testing - Add 6 unit tests covering mergePagePreservingIdentity edge cases
1 parent 31e85fb commit 0396f3f

3 files changed

Lines changed: 140 additions & 28 deletions

File tree

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

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1653,9 +1653,7 @@ export function TableGrid({
16531653
if (undoCells.length > 0) {
16541654
pushUndoRef.current({ type: 'clear-cells', cells: undoCells })
16551655
}
1656-
if (batchUpdates.length > 0) {
1657-
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
1658-
}
1656+
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
16591657
})().catch((error) => {
16601658
logger.error('Failed to clear selected cells', { error })
16611659
toast.error('Failed to clear cells — please try again')
@@ -1882,8 +1880,7 @@ export function TableGrid({
18821880
batchUpdates.push({ rowId: row.id, data: updates })
18831881
}
18841882
if (undoCells.length > 0) pushUndoRef.current({ type: 'clear-cells', cells: undoCells })
1885-
if (batchUpdates.length > 0)
1886-
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
1883+
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
18871884
})().catch((error) => {
18881885
logger.error('Failed to clear column values', { error })
18891886
toast.error('Failed to clear column values — please try again')
@@ -1908,12 +1905,10 @@ export function TableGrid({
19081905
undoCells.push({ rowId: row.id, data: previousData })
19091906
batchUpdates.push({ rowId: row.id, data: updates })
19101907
}
1911-
if (batchUpdates.length > 0) {
1912-
void chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current).catch((error) => {
1913-
logger.error('Failed to clear selected cells', { error })
1914-
toast.error('Failed to clear cells — please try again')
1915-
})
1916-
}
1908+
void chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current).catch((error) => {
1909+
logger.error('Failed to clear selected cells', { error })
1910+
toast.error('Failed to clear cells — please try again')
1911+
})
19171912
if (undoCells.length > 0) {
19181913
pushUndoRef.current({ type: 'clear-cells', cells: undoCells })
19191914
}
@@ -1991,6 +1986,51 @@ export function TableGrid({
19911986
if (!sel) return
19921987

19931988
e.preventDefault()
1989+
1990+
if (isColumnSelectionRef.current) {
1991+
// Column-header copy spans all rows — drain pages first, then use async
1992+
// clipboard so we don't block the event before the drain completes.
1993+
void (async () => {
1994+
const allRows = await ensureAllRowsLoadedRef.current()
1995+
const lines: string[] = []
1996+
for (const row of allRows) {
1997+
const cells: string[] = []
1998+
for (let c = sel.startCol; c <= sel.endCol; c++) {
1999+
const colName = cols[c]?.name
2000+
if (!colName) continue
2001+
const value: unknown = row.data[colName]
2002+
cells.push(
2003+
value === null || value === undefined
2004+
? ''
2005+
: typeof value === 'object'
2006+
? JSON.stringify(value)
2007+
: String(value)
2008+
)
2009+
}
2010+
lines.push(cells.join('\t'))
2011+
}
2012+
if (!navigator.clipboard) {
2013+
toast.error('Clipboard access is unavailable in this context')
2014+
return
2015+
}
2016+
try {
2017+
await navigator.clipboard.writeText(lines.join('\n'))
2018+
} catch (err) {
2019+
if (err instanceof DOMException && err.name === 'NotAllowedError') {
2020+
toast.error(
2021+
'Clipboard permission expired — press Cmd+C again immediately after selecting'
2022+
)
2023+
} else {
2024+
throw err
2025+
}
2026+
}
2027+
})().catch((error) => {
2028+
logger.error('Failed to copy column cells', { error })
2029+
toast.error('Failed to copy — please try again')
2030+
})
2031+
return
2032+
}
2033+
19942034
const lines: string[] = []
19952035
for (let r = sel.startRow; r <= sel.endRow; r++) {
19962036
const cells: string[] = []
@@ -2127,9 +2167,7 @@ export function TableGrid({
21272167
if (undoCells.length > 0) {
21282168
pushUndoRef.current({ type: 'clear-cells', cells: undoCells })
21292169
}
2130-
if (batchUpdates.length > 0) {
2131-
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
2132-
}
2170+
await chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current)
21332171
})().catch((error) => {
21342172
logger.error('Failed to cut column cells', { error })
21352173
toast.error('Failed to cut — please try again')
@@ -2164,12 +2202,10 @@ export function TableGrid({
21642202
batchUpdates.push({ rowId: row.id, data: updates })
21652203
}
21662204
e.clipboardData?.setData('text/plain', lines.join('\n'))
2167-
if (batchUpdates.length > 0) {
2168-
void chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current).catch((error) => {
2169-
logger.error('Failed to cut selected cells', { error })
2170-
toast.error('Failed to cut — please try again')
2171-
})
2172-
}
2205+
void chunkBatchUpdates(batchUpdates, batchUpdateAsyncRef.current).catch((error) => {
2206+
logger.error('Failed to cut selected cells', { error })
2207+
toast.error('Failed to cut — please try again')
2208+
})
21732209
if (undoCells.length > 0) {
21742210
pushUndoRef.current({ type: 'clear-cells', cells: undoCells })
21752211
}

apps/sim/hooks/queries/tables.test.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ vi.mock('@/components/emcn', () => ({
8282
}))
8383

8484
import {
85+
_mergePagePreservingIdentity,
8586
tableKeys,
8687
tableRowsInfiniteOptions,
8788
tableRowsParamsKey,
@@ -370,3 +371,75 @@ describe('tableRowsInfiniteOptions', () => {
370371
expect(JSON.stringify(opts1.queryKey)).not.toBe(JSON.stringify(opts2.queryKey))
371372
})
372373
})
374+
375+
describe('_mergePagePreservingIdentity', () => {
376+
const ts = '2024-01-01T00:00:00.000Z'
377+
const ts2 = '2024-01-02T00:00:00.000Z'
378+
379+
function makeRow(id: string, updatedAt: string, extra?: Record<string, unknown>) {
380+
return { id, updatedAt, data: { value: id, ...extra } }
381+
}
382+
383+
it('returns fresh when totalCount differs', () => {
384+
const prev = { rows: [makeRow('r1', ts)], totalCount: 1, nextOffset: undefined }
385+
const fresh = { rows: [makeRow('r1', ts)], totalCount: 2, nextOffset: undefined }
386+
const result = _mergePagePreservingIdentity(prev, fresh)
387+
expect(result).toBe(fresh)
388+
})
389+
390+
it('returns fresh when row counts differ', () => {
391+
const prev = { rows: [makeRow('r1', ts)], totalCount: 2, nextOffset: undefined }
392+
const fresh = {
393+
rows: [makeRow('r1', ts), makeRow('r2', ts)],
394+
totalCount: 2,
395+
nextOffset: undefined,
396+
}
397+
const result = _mergePagePreservingIdentity(prev, fresh)
398+
expect(result).toBe(fresh)
399+
})
400+
401+
it('returns prev (same reference) when all rows are unchanged', () => {
402+
const row1 = makeRow('r1', ts)
403+
const row2 = makeRow('r2', ts)
404+
const prev = { rows: [row1, row2], totalCount: 2, nextOffset: undefined }
405+
const freshRow1 = makeRow('r1', ts)
406+
const fresh = { rows: [freshRow1, makeRow('r2', ts)], totalCount: 2, nextOffset: undefined }
407+
const result = _mergePagePreservingIdentity(prev, fresh)
408+
expect(result).toBe(prev)
409+
})
410+
411+
it('preserves identity for unchanged rows, uses fresh for updated rows', () => {
412+
const row1 = makeRow('r1', ts)
413+
const row2 = makeRow('r2', ts)
414+
const prev = { rows: [row1, row2], totalCount: 2, nextOffset: undefined }
415+
const updatedRow2 = makeRow('r2', ts2, { extra: 'new' })
416+
const fresh = { rows: [makeRow('r1', ts), updatedRow2], totalCount: 2, nextOffset: undefined }
417+
const result = _mergePagePreservingIdentity(prev, fresh)
418+
expect(result).not.toBe(prev)
419+
expect(result.rows[0]).toBe(row1)
420+
expect(result.rows[1]).toBe(updatedRow2)
421+
})
422+
423+
it('uses fresh row when ID is not found in prev', () => {
424+
const row1 = makeRow('r1', ts)
425+
const prev = { rows: [row1, makeRow('r2', ts)], totalCount: 2, nextOffset: undefined }
426+
const newRow = makeRow('r3', ts)
427+
const fresh = { rows: [makeRow('r1', ts), newRow], totalCount: 2, nextOffset: undefined }
428+
const result = _mergePagePreservingIdentity(prev, fresh)
429+
expect(result.rows[1]).toBe(newRow)
430+
})
431+
432+
it('compares updatedAt as dates, not strings (ISO vs different string forms)', () => {
433+
const row1 = makeRow('r1', ts)
434+
const prev = { rows: [row1], totalCount: 1, nextOffset: undefined }
435+
// Same point in time, different ISO string representation (with trailing Z vs +00:00)
436+
const sameTimeDifferentFormat = '2024-01-01T00:00:00+00:00'
437+
const fresh = {
438+
rows: [makeRow('r1', sameTimeDifferentFormat)],
439+
totalCount: 1,
440+
nextOffset: undefined,
441+
}
442+
const result = _mergePagePreservingIdentity(prev, fresh)
443+
expect(result).toBe(prev)
444+
})
445+
})

apps/sim/hooks/queries/tables.ts

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -251,11 +251,8 @@ export function useTableRows({
251251
})
252252
}
253253

254-
/** Merges a freshly-fetched page into the cached page while preserving row
255-
* object identity for unchanged rows (by `updatedAt`). Returns the original
256-
* `prev` reference unchanged when nothing has actually changed — callers can
257-
* use a `===` check to skip a `setQueryData` write entirely. */
258-
function mergePagePreservingIdentity(
254+
/** @internal — exported for testing only. */
255+
export function _mergePagePreservingIdentity(
259256
prev: TableRowsResponse,
260257
fresh: TableRowsResponse
261258
): TableRowsResponse {
@@ -264,7 +261,8 @@ function mergePagePreservingIdentity(
264261
let allSame = true
265262
const nextRows = fresh.rows.map((freshRow) => {
266263
const prevRow = prevById.get(freshRow.id)
267-
if (prevRow && String(prevRow.updatedAt) === String(freshRow.updatedAt)) return prevRow
264+
if (prevRow && new Date(prevRow.updatedAt).getTime() === new Date(freshRow.updatedAt).getTime())
265+
return prevRow
268266
allSame = false
269267
return freshRow
270268
})
@@ -343,7 +341,12 @@ export function useInfiniteTableRows({
343341
const tick = async () => {
344342
if (cancelled) return
345343
let hasDirty = false
346-
if (queryClient.isMutating() === 0) {
344+
if (queryClient.isMutating() !== 0) {
345+
// Mutation in progress — skip network fetch to avoid racing optimistic
346+
// updates, but stay on the short interval so we catch up quickly once
347+
// the mutation settles.
348+
hasDirty = true
349+
} else {
347350
const data = queryClient.getQueryData<InfiniteData<TableRowsResponse, number>>(queryKey)
348351
const dirty: number[] = []
349352
if (data) {
@@ -374,7 +377,7 @@ export function useInfiniteTableRows({
374377
if (!prev) return prev
375378
const idx = prev.pageParams.indexOf(offset)
376379
if (idx === -1) return prev
377-
const merged = mergePagePreservingIdentity(prev.pages[idx], fresh)
380+
const merged = _mergePagePreservingIdentity(prev.pages[idx], fresh)
378381
if (merged === prev.pages[idx]) return prev
379382
const nextPages = prev.pages.slice()
380383
nextPages[idx] = merged

0 commit comments

Comments
 (0)