feat(tables): virtualize data grid with bounded copy and chunked delete#4693
feat(tables): virtualize data grid with bounded copy and chunked delete#4693TheodoreSpeaks wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Large-selection correctness: adds Reviewed by Cursor Bugbot for commit 8956ef5. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR virtualizes the user-tables data grid using
Confidence Score: 3/5The virtualization and chunked-delete paths look mechanically sound, but the clipboard error handler can actively mislead users when failures are unrelated to table size. The catch handler in writeSelectionToClipboard always shows 'Selection too large to copy — use Export CSV' regardless of the actual failure. A network error during row paging, or a server error while clearing cut cells, produces that same message — pointing users toward Export CSV when no size limit was involved. For cut, this is particularly confusing because the data landed on the clipboard successfully before the clear failed. apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx — specifically the writeSelectionToClipboard catch handler and the stale loading toast. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TableGrid
participant ClipboardAPI
participant QueryCache
participant Server
Note over TableGrid: Copy/Cut (Cmd+C / Cmd+X)
User->>TableGrid: keyboard event (copy/cut)
TableGrid->>ClipboardAPI: "clipboard.write([ClipboardItem({blob_promise})])"
Note over TableGrid,ClipboardAPI: Transient activation captured synchronously
par Blob resolution (async)
TableGrid->>QueryCache: ensureRowsLoadedUpTo(50_000)
loop "while loadedCount < maxRows && lastPage full"
QueryCache->>Server: fetchNextPage()
Server-->>QueryCache: page of up to 1000 rows
end
QueryCache-->>TableGrid: "{rows, hasMore}"
TableGrid->>TableGrid: serialize rows to Blob
TableGrid-->>ClipboardAPI: Blob resolved
end
ClipboardAPI-->>TableGrid: write() resolved or rejected
alt success
TableGrid->>TableGrid: afterCopy?(copiedRows)
opt Cut operation
TableGrid->>Server: chunkBatchUpdates (clear cells)
end
TableGrid->>User: toast Copied N rows or first 50k
else any error
TableGrid->>User: toast Selection too large use Export CSV
end
Note over TableGrid: Delete All (context menu)
User->>TableGrid: Delete rows (select-all)
TableGrid->>QueryCache: ensureAllRowsLoaded()
loop until last page partial
QueryCache->>Server: fetchNextPage()
Server-->>QueryCache: page
end
QueryCache-->>TableGrid: all rows
TableGrid->>Server: deleteRows chunked up to 1000 per request sequential
Reviews (1): Last reviewed commit: "feat(tables): virtualize data grid with ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .catch((error) => { | ||
| logger.error(`Failed to ${opts.verb === 'Copied' ? 'copy' : 'cut'} rows`, { error }) | ||
| toast.error('Selection too large to copy — use Export CSV') | ||
| }) |
There was a problem hiding this comment.
The .catch() handler unconditionally shows "Selection too large to copy — use Export CSV" for every failure, including two cases where the message is factually wrong:
- Network error during row loading — if
opts.loadRows()rejects (e.g., a transient HTTP 500 while paging), the blob promise is rejected,clipboard.write()rejects with that error, and the user sees "Selection too large" for a network issue—especially confusing when only a handful of rows are selected. - Cut-clear failure after a successful copy —
clearCutRowsruns inside.then()viaafterCopy; ifchunkBatchUpdatesfails mid-cut, the data is already on the clipboard but.then()throws, which falls to this same.catch(). The user sees "Selection too large" when the real problem is that the server-side cell clear failed.
The previous code at minimum distinguished NotAllowedError from other failures; this PR collapses all errors into a single misleading message. At minimum the catch should differentiate between a size-cap scenario and any other error.
| const all = data?.pages.flatMap((p) => p.rows) ?? [] | ||
| const lastPage = data?.pages[data.pages.length - 1] | ||
| const morePages = lastPage ? lastPage.rows.length === TABLE_LIMITS.MAX_QUERY_LIMIT : false | ||
| return { | ||
| rows: all.length > maxRows ? all.slice(0, maxRows) : all, | ||
| hasMore: morePages || all.length > maxRows, | ||
| } |
There was a problem hiding this comment.
morePages heuristic produces a false-positive when the table row count equals MAX_COPY_ROWS exactly
morePages is set to true whenever the last fetched page contains exactly MAX_QUERY_LIMIT (1 000) rows. When a table has exactly 50 000 rows the loop fetches all 50 pages and breaks because loadedCount() >= maxRows. At that point lastPage.rows.length === 1000 so morePages = true, but all.length === maxRows so the all.length > maxRows guard is also false — resulting in hasMore: true even though every row was returned. The clipboard code then shows "Copied first 50 000 rows — export to CSV for the rest" when the copy was actually complete.
| toast.error('Clipboard access is unavailable in this context') | ||
| return | ||
| } | ||
| toast({ message: `${opts.verb === 'Copied' ? 'Copying' : 'Cutting'}… loading rows` }) |
There was a problem hiding this comment.
toast({ message: 'Copying… loading rows' }) is fired synchronously but there is no handle to dismiss or replace it once the operation finishes. When clipboard.write() resolves or rejects, a second toast is shown. Depending on the toast queue configuration this leaves both toasts visible simultaneously, which could confuse users on fast operations where the "loading" flash and "Copied N rows" appear back-to-back or overlap.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8956ef5. Configure here.
| .catch((error) => { | ||
| logger.error(`Failed to ${opts.verb === 'Copied' ? 'copy' : 'cut'} rows`, { error }) | ||
| toast.error('Selection too large to copy — use Export CSV') | ||
| }) |
There was a problem hiding this comment.
Cut cell-clearing failure shows misleading clipboard error
Medium Severity
The .catch() handler in writeSelectionToClipboard catches errors from both clipboard.write() and the afterCopy callback (i.e., clearCutRows). When a cut operation's cell-clearing fails (e.g., a chunkBatchUpdates network error), the toast displays "Selection too large to copy — use Export CSV" even though the clipboard write already succeeded. The old code correctly showed "Failed to cut — please try again" for this scenario. Additionally, the undo entry inside clearCutRows is pushed before the batch update, so on failure the undo stack has a dangling entry while the clipboard silently holds the data.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8956ef5. Configure here.
1 participant
Summary
@tanstack/react-virtualspacer-row approach over the native table) so 4k+ row tables scroll smoothly — DOM goes from one<tr>per loaded row to visible + overscan. Pages kept unbounded so bulk ops keep working; memory drops.ClipboardItem(fixes clipboard gesture-expiry); capped at 50k rows with a "use Export CSV" steer beyond that. Cut clears only the copied rows.Type of Change
Testing
Tested manually.
tsc --noEmitclean,vitest hooks/queries lib/table(202 passing),bun run lint(16/16), andcheck:api-validation:strictall pass.Checklist