feat(tables): paginated background row-delete jobs via table_jobs#4915
feat(tables): paginated background row-delete jobs via table_jobs#4915TheodoreSpeaks wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Import state moves off Grid UX: select-all supports an exclusion set, filter-scoped totals for counts, async delete with optimistic row removal and confirmation; Cmd+A toggles whole-table selection. Cron janitor marks stale running jobs failed and prunes old terminal job rows. Reviewed by Cursor Bugbot for commit b324ac0. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds a background row-delete job (paginated, filter-scoped, ownership-gated) alongside a new
Confidence Score: 4/5Safe to merge after addressing the incorrect row count shown in the delete confirmation dialog when a filter is active. The backend (migration, delete runner, job CRUD, concurrency gate) is well-designed and correct. The one defect is in the frontend: the delete confirmation dialog shows the total table row count instead of the filtered count when a filter is active, so a user deleting 50 filtered rows sees 'Delete 10,000 rows…'. The over-broad optimistic cache clear is a minor multi-cached-state edge case. Everything else — keyset pagination, cutoff semantics, ownership checks, SSE reconciliation, import compatibility — looks solid. apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx (estimatedCount logic) and apps/sim/hooks/queries/tables.ts (onMutate query scope) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant DeleteAsyncRoute as POST /delete-async
participant DB as PostgreSQL
participant Worker as runTableDelete (detached)
participant SSE as SSE stream
Client->>DeleteAsyncRoute: "POST {filter, excludeRowIds}"
DeleteAsyncRoute->>DB: markTableJobRunning (INSERT table_jobs ON CONFLICT DO NOTHING)
alt slot already taken
DB-->>DeleteAsyncRoute: conflict 0 rows
DeleteAsyncRoute-->>Client: 409 A job is already in progress
else slot claimed
DB-->>DeleteAsyncRoute: job row inserted
DeleteAsyncRoute->>Worker: runDetached(runTableDelete)
DeleteAsyncRoute-->>Client: "200 {jobId}"
loop each page
Worker->>DB: updateJobProgress (heartbeat + ownership check)
alt job canceled
DB-->>Worker: 0 rows JobSupersededError stop
end
Worker->>DB: "selectRowIdPage (id > afterId, createdAt <= cutoff, filter)"
DB-->>Worker: page of IDs
Worker->>DB: deletePageByIds skipCompaction
Worker->>SSE: appendTableEvent(running, progress) every 5000 rows
end
Worker->>DB: markJobReady
Worker->>SSE: appendTableEvent(ready)
SSE-->>Client: invalidate rows + detail query
end
opt user cancels
Client->>DB: "POST /job/cancel {jobId}"
DB-->>Client: "canceled=true"
SSE-->>Client: appendTableEvent(canceled) restore optimistic rows
end
Reviews (1): Last reviewed commit: "feat(tables): paginated background row-d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const excludeRowIds = rowSel.excluded ? [...rowSel.excluded] : [] | ||
| const estimatedCount = Math.max(0, tableRowCountRef.current - excludeRowIds.length) | ||
| onRequestDeleteAllByFilter({ excludeRowIds, estimatedCount }) |
There was a problem hiding this comment.
estimatedCount uses tableRowCountRef.current, which is the TOTAL table row count (tableData.rowCount), not the count matching the active filter. When a filter is active, the confirmation dialog will say "Delete 10,000 rows matching the current filter" even though only a small subset of those rows match the filter and will actually be deleted. This could alarm users or make them distrust the UI. The filtered count should come from the rows query's metadata (or at minimum the dialog should not show a count when a filter is active).
| const excludeRowIds = rowSel.excluded ? [...rowSel.excluded] : [] | |
| const estimatedCount = Math.max(0, tableRowCountRef.current - excludeRowIds.length) | |
| onRequestDeleteAllByFilter({ excludeRowIds, estimatedCount }) | |
| const excludeRowIds = rowSel.excluded ? [...rowSel.excluded] : [] | |
| // When a filter is active, tableRowCountRef.current is the *total* table count, not the | |
| // filtered count — pass undefined so the dialog avoids showing a misleading number. | |
| const estimatedCount = queryOptions?.filter | |
| ? undefined | |
| : Math.max(0, tableRowCountRef.current - excludeRowIds.length) | |
| onRequestDeleteAllByFilter({ excludeRowIds, estimatedCount }) |
| await queryClient.cancelQueries({ queryKey: tableKeys.rowsRoot(tableId) }) | ||
| const previousQueries = queryClient.getQueriesData<InfiniteData<TableRowsResponse, number>>({ | ||
| queryKey: tableKeys.rowsRoot(tableId), | ||
| }) | ||
| const previousDetail = queryClient.getQueryData<TableDefinition>(tableKeys.detail(tableId)) | ||
| const keep = new Set(excludeRowIds ?? []) | ||
| queryClient.setQueriesData<InfiniteData<TableRowsResponse, number>>( | ||
| { queryKey: tableKeys.rowsRoot(tableId), exact: false }, | ||
| (old) => | ||
| old | ||
| ? { | ||
| ...old, | ||
| pages: old.pages.map((page) => ({ | ||
| ...page, | ||
| rows: page.rows.filter((r) => keep.has(r.id)), | ||
| })), | ||
| } | ||
| : old |
There was a problem hiding this comment.
setQueriesData with exact: false updates every cached row query for this table, not just the one that matches the current filter. If the user previously loaded the table without a filter (or with a different filter), those cached pages are also cleared. Since onSuccess only invalidates tableKeys.lists() (not row queries), the stale no-op rows stay empty until the SSE terminal event fires — which may be minutes later for a large delete. The fix is to restrict the optimistic update to the exact query key that matches the current filter.
…ed optimistic clear, Cmd+A select-all, hide delete from tray)
…row-deletes # Conflicts: # apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx # apps/sim/app/workspace/[workspaceId]/tables/components/import-csv-dialog/import-csv-dialog.tsx # apps/sim/app/workspace/[workspaceId]/tables/tables.tsx # apps/sim/lib/table/import-runner.ts # apps/sim/lib/table/service.ts # packages/db/migrations/meta/0227_snapshot.json # packages/db/migrations/meta/_journal.json
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b324ac0. Configure here.
| if (excluded.has(targetId)) excluded.delete(targetId) | ||
| else excluded.add(targetId) | ||
| return { kind: 'all', excluded: excluded.size === 0 ? undefined : excluded } | ||
| } |
There was a problem hiding this comment.
Shift-click breaks select-all
Medium Severity
After a filter-scoped select-all (kind: 'all'), a shift-click on a row checkbox skips the exclusion path and calls rowSelectionMaterialize, which collapses the selection to loaded row ids only. Unloaded rows that still match the filter drop out of the selection and from async delete scope.
Reviewed by Cursor Bugbot for commit b324ac0. Configure here.
| const lockedId = prev?.importId | ||
| if (lockedId && importId && importId !== lockedId) return | ||
| const lockedId = prev?.jobId | ||
| if (lockedId && jobId && jobId !== lockedId) return |
There was a problem hiding this comment.
Terminal job events dropped early
High Severity
applyJob ignores terminal SSE events when the table detail cache has no jobId, but async delete clears rows in onMutate and only sets jobId in onSuccess. If the worker finishes and emits ready or failed before that lands, the event is dropped, row invalidation never runs, and a failed delete can leave the grid empty while data still exists.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b324ac0. Configure here.
1 participant
Summary
{ filter, excludeRowIds }instead of every row id, deleting in keyset-paginated batches. Rows inserted mid-job are spared via acreated_atcutoff.table_jobstable (one row per job,type= import|delete). Concurrency gate is a partial-unique index on(table_id) WHERE status='running'— one bg job per table, so import/delete are mutually exclusive. Job fields are re-exposed onTableDefinitionvia a latest-job join, so the tray/SSE/grid stay unchanged.{ kind:'all', excluded }so "select all then deselect" maps cleanly to the job — fixes the old bug where deselect-after-select-all silently dropped unloaded rows.0227: createtable_jobs, backfill in-flight import state, drop the oldimport_*columns. Janitor sweeps stale running jobs → failed and prunes terminal jobs.Type of Change
Testing
bun run lintclean,bun run check:api-validation:strictpasses,tsc --noEmitclean, 260 table/cron tests pass (added delete-runner, delete-async, and job/cancel tests).Checklist