Skip to content

Commit 28a26c1

Browse files
authored
improvement(tables): fire the live-rows signal on async delete, run cancel, and column run (#6094)
* improvement(tables): fire the live-rows signal on async delete, run cancel, and column run These three table operations mutate row data but emitted no `rows` change signal, so open editors' grids stayed stale until a manual refresh (enrichment *results* already stream live via `cell` events; these are the bulk paths that don't emit per-cell events): - Async row delete (`runTableDelete`): signal as rows drop out (throttled with the existing progress event) and once more on completion — the `job` progress event only drives the delete meter, not the rows query. Covers the delete-async route and the copilot bulk-delete, since both share the runner. - Cancel runs (`cancel-runs` route): cancelling clears each affected row's exec state; the `dispatch: cancelled` events drop the run overlay but the client then renders authoritative DB state, so refetch. Only when something was actually cancelled. - Run column (`columns/run` route): starting a run bulk-clears the target group's cells to pending; refetch so the cleared cells show. Only when a dispatch was actually created. Guarded so no signal fires on a no-op/failure. Adds a delete-runner test asserting the completion signal. * fix(tables): guarantee the live-rows signal on every mutating path (review) - Delete runner (Greptile P1): a batch could commit and the job then cancel/supersede before the next throttled progress signal or `markJobReady`, bypassing both signals and leaving deleted rows on screen. Track `deletedAny` and fire the grid refetch in a `finally`, so it runs on EVERY exit — completion, cancel/supersede, mid-batch lock, or a rethrown error after a partial delete. - cancel-runs / columns/run routes (Cursor): the `cancelled > 0` / `if (dispatchId)` guards don't always reflect DB row changes — cancel tombstones exec state even when 0 dispatches were active, and a run bulk-clears cells then can return a null dispatchId. Signal unconditionally; a stale-but-harmless refetch beats a missed one. - Tests: assert the delete signal fires on the mid-run-cancel-after-delete path and NOT when nothing was deleted. * fix(tables): mark deletedAny before the page delete so a mid-page lock still refreshes the grid `deletePageByIds` commits in internal batches, so a delete lock landing mid-page can persist earlier batches and THEN throw TableLockedError — the catch returns without a count, so setting `deletedAny` from the return value missed it and the finally skipped the grid refetch. Set `deletedAny = true` before the call (any attempt may commit rows); an attempt that commits nothing only over-refetches (harmless). Adds a test asserting the signal fires when a page throws a mid-page lock.
1 parent 74ca851 commit 28a26c1

4 files changed

Lines changed: 62 additions & 2 deletions

File tree

apps/sim/app/api/table/[tableId]/cancel-runs/route.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { parseRequest } from '@/lib/api/server'
55
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
66
import { generateRequestId } from '@/lib/core/utils/request'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
8+
import { signalTableRowsChanged } from '@/lib/table/events'
89
import { cancelWorkflowGroupRuns } from '@/lib/table/workflow-columns'
910
import { accessError, checkAccess, tableFilterError } from '@/app/api/table/utils'
1011

@@ -55,6 +56,12 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
5556
} cancelled=${cancelled}`
5657
)
5758

59+
// Cancelling clears/tombstones affected rows' exec state in the DB. The `dispatch: cancelled` events
60+
// drop the run overlay, but the client then renders the row's authoritative DB state — so refetch the
61+
// grid to pick up the cleared cells. Unconditional: `cancelled` counts dispatches, but tombstone row
62+
// writes can happen even when that is 0, and a stale-but-harmless refetch beats a missed one.
63+
signalTableRowsChanged(tableId)
64+
5865
return NextResponse.json({ success: true, data: { cancelled } })
5966
} catch (error) {
6067
logger.error(`[${requestId}] cancel-runs failed:`, error)

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { parseRequest } from '@/lib/api/server'
55
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
66
import { generateRequestId } from '@/lib/core/utils/request'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
8+
import { signalTableRowsChanged } from '@/lib/table/events'
89
import { runWorkflowColumn } from '@/lib/table/workflow-columns'
910
import { accessError, checkAccess, tableFilterError } from '@/app/api/table/utils'
1011

@@ -47,6 +48,13 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
4748
triggeredByUserId: auth.userId,
4849
})
4950

51+
// Starting a run clears the target group's cells to pending (`bulkClearWorkflowGroupCells`) — a DB
52+
// row change. The `dispatch: dispatching` events drive the run overlay, but the cleared cell values
53+
// come from the rows query, so refetch the grid. Unconditional: the bulk clear can run even on a path
54+
// that then returns a null `dispatchId` (dispatch cancelled post-clear), and a stale-but-harmless
55+
// refetch beats a missed one.
56+
signalTableRowsChanged(tableId)
57+
5058
return NextResponse.json({ success: true, data: { dispatchId } })
5159
} catch (error) {
5260
if (error instanceof Error && error.message === 'Invalid workspace ID') {

apps/sim/lib/table/delete-runner.test.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* @vitest-environment node
33
*/
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
import { TableLockedError } from '@/lib/table/mutation-locks'
56

67
const {
78
mockGetTableById,
@@ -13,6 +14,7 @@ const {
1314
mockMarkJobFailed,
1415
mockMarkJobCanceled,
1516
mockAppendTableEvent,
17+
mockSignalTableRowsChanged,
1618
mockBuildFilterClause,
1719
} = vi.hoisted(() => ({
1820
mockGetTableById: vi.fn(),
@@ -24,6 +26,7 @@ const {
2426
mockMarkJobFailed: vi.fn(),
2527
mockMarkJobCanceled: vi.fn(),
2628
mockAppendTableEvent: vi.fn(),
29+
mockSignalTableRowsChanged: vi.fn(),
2730
mockBuildFilterClause: vi.fn(),
2831
}))
2932

@@ -41,7 +44,10 @@ vi.mock('@/lib/table/rows/ordering', () => ({
4144
selectRowIdPage: mockSelectRowIdPage,
4245
deletePageByIds: mockDeletePageByIds,
4346
}))
44-
vi.mock('@/lib/table/events', () => ({ appendTableEvent: mockAppendTableEvent }))
47+
vi.mock('@/lib/table/events', () => ({
48+
appendTableEvent: mockAppendTableEvent,
49+
signalTableRowsChanged: mockSignalTableRowsChanged,
50+
}))
4551
vi.mock('@/lib/table/sql', () => ({ buildFilterClause: mockBuildFilterClause }))
4652
vi.mock('@/lib/table/constants', () => ({
4753
TABLE_LIMITS: { DELETE_PAGE_SIZE: 2 },
@@ -89,6 +95,8 @@ describe('runTableDelete', () => {
8995
expect(mockAppendTableEvent).toHaveBeenCalledWith(
9096
expect.objectContaining({ kind: 'job', type: 'delete', status: 'canceled' })
9197
)
98+
// Nothing was deleted, so the grid must NOT be needlessly refetched.
99+
expect(mockSignalTableRowsChanged).not.toHaveBeenCalled()
92100
})
93101

94102
it('stops mid-run when the delete lock is enabled between pages', async () => {
@@ -111,6 +119,21 @@ describe('runTableDelete', () => {
111119
)
112120
expect(mockMarkJobCanceled).toHaveBeenCalledWith('tbl_1', 'job_1')
113121
expect(mockMarkJobReady).not.toHaveBeenCalled()
122+
// Even though the run was cancelled before completion, the first page WAS deleted — the `finally`
123+
// must still refetch the grid so open editors don't keep showing those deleted rows.
124+
expect(mockSignalTableRowsChanged).toHaveBeenCalledWith('tbl_1')
125+
})
126+
127+
it('signals a grid refetch when a page throws a mid-page lock after committing rows', async () => {
128+
mockSelectRowIdPage.mockResolvedValueOnce(['a', 'b'])
129+
// `deletePageByIds` commits in internal batches, so a lock landing mid-page can persist earlier
130+
// batches and THEN throw — it returns no count. The grid must still be refetched.
131+
mockDeletePageByIds.mockRejectedValueOnce(new TableLockedError('delete'))
132+
133+
await expect(runTableDelete(basePayload())).resolves.toBeUndefined()
134+
135+
expect(mockMarkJobCanceled).toHaveBeenCalledWith('tbl_1', 'job_1')
136+
expect(mockSignalTableRowsChanged).toHaveBeenCalledWith('tbl_1')
114137
})
115138

116139
it('deletes every matching page then marks the job ready', async () => {
@@ -141,6 +164,9 @@ describe('runTableDelete', () => {
141164
expect(mockAppendTableEvent).toHaveBeenCalledWith(
142165
expect.objectContaining({ kind: 'job', type: 'delete', status: 'ready', progress: 3 })
143166
)
167+
// The live grid must be told rows changed so deleted rows drop out of every open editor —
168+
// the `job` progress event only drives the delete meter, not the rows query.
169+
expect(mockSignalTableRowsChanged).toHaveBeenCalledWith('tbl_1')
144170
})
145171

146172
it('stops once maxRows is reached and caps the final page fetch to the remaining budget', async () => {

apps/sim/lib/table/delete-runner.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { generateId } from '@sim/utils/id'
44
import { truncate } from '@sim/utils/string'
55
import type { Filter, TableDefinition } from '@/lib/table'
66
import { TABLE_LIMITS, USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
7-
import { appendTableEvent } from '@/lib/table/events'
7+
import { appendTableEvent, signalTableRowsChanged } from '@/lib/table/events'
88
import {
99
getJobProgress,
1010
markJobCanceled,
@@ -65,6 +65,12 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
6565
const requestId = generateId().slice(0, 8)
6666
const budget = maxRows ?? Number.POSITIVE_INFINITY
6767

68+
// Whether any row was actually deleted this run. Signalled in `finally` so open editors refetch the
69+
// grid on EVERY exit path — normal completion, a cancel/supersede between batches, a mid-batch lock,
70+
// or a rethrown error after a partial delete — not only the throttled/`ready` paths (which a cancel
71+
// landing after a committed batch would bypass, leaving deleted rows on screen).
72+
let deletedAny = false
73+
6874
try {
6975
const table = await getTableById(tableId, { includeArchived: true })
7076
if (!table) throw new Error(`Delete target table ${tableId} not found`)
@@ -153,6 +159,11 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
153159

154160
const toDelete = excluded.size > 0 ? page.filter((id) => !excluded.has(id)) : page
155161
if (toDelete.length > 0) {
162+
// Mark BEFORE the call, not from its return: `deletePageByIds` commits in internal batches, so a
163+
// mid-page lock can persist earlier batches and THEN throw — the catch below returns without a
164+
// count. Setting this up front guarantees the `finally` grid refetch fires whether the call
165+
// returns or throws. (An attempt that ends up committing nothing only over-refetches — harmless.)
166+
deletedAny = true
156167
try {
157168
processed += await deletePageByIds(tableId, workspaceId, toDelete, pageProof, revalidate)
158169
} catch (err) {
@@ -178,6 +189,10 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
178189
status: 'running',
179190
progress: processed,
180191
})
192+
// Refetch the live grid as rows drop out (throttled with the progress event above) — the `job`
193+
// event only drives the progress meter, not the rows query. The `finally` below guarantees a
194+
// final refetch on every exit, so a cancel after the last un-throttled batch can't leave stale rows.
195+
signalTableRowsChanged(tableId)
181196
}
182197
}
183198

@@ -217,6 +232,10 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
217232
const error = cause ? toError(cause) : toError(err)
218233
logger.error(`[${requestId}] Delete failed for table ${tableId}:`, error)
219234
throw error
235+
} finally {
236+
// Guaranteed final grid refetch on every exit — completion, cancel/supersede, mid-batch lock, or a
237+
// rethrown error — whenever this run deleted anything, so no open editor keeps showing deleted rows.
238+
if (deletedAny) signalTableRowsChanged(tableId)
220239
}
221240
}
222241

0 commit comments

Comments
 (0)