fix(tables): stop every table paginating forever on a null totalCount - #6694
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Pagination threads Server-side: filtered row counts treat an active delete mask as a filter (tenant-bounded count path); read transaction guards ( Reviewed by Cursor Bugbot for commit 9861887. Configure here. |
Greptile SummaryThe PR fixes table pagination and filtered totals by accepting boolean query flags, retaining the server’s authoritative cursor, and consolidating transaction-local read guards.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/hooks/queries/tables.ts | Retains the server cursor in cached pages and terminates optimistic async-delete views explicitly. |
| apps/sim/hooks/queries/utils/table-rows-pagination.ts | Makes the last page’s server cursor authoritative while retaining count-based compatibility for legacy cached pages. |
| apps/sim/lib/api/contracts/tables.ts | Reuses the shared boolean query-flag schema so internal boolean values and canonical URL values parse correctly. |
| apps/sim/lib/table/planner.ts | Consolidates transaction-local timeout and optional planner settings into one valid set_config query. |
| apps/sim/lib/table/rows/service.ts | Treats active deletion masks as filtered count queries and documents the pagination batch-limit invariant. |
| apps/sim/hooks/queries/utils/table-rows-pagination.test.ts | Covers cursor precedence, last-page selection, legacy fallback, and optimistic deletion termination. |
| apps/sim/lib/api/contracts/tables.test.ts | Covers boolean, URL-string, absent, and empty includeTotal inputs. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Grid[Table grid] -->|GET rows, includeTotal on page 0| Contract[Table rows contract]
Contract --> Service[Row query service]
Service --> Guards[Transaction-local read guards]
Guards --> DB[(Postgres)]
DB --> Service
Service -->|rows, totalCount, nextCursor| Grid
Grid --> Decision{Last-page nextCursor}
Decision -->|non-null| Fetch[Fetch next page]
Decision -->|null| Stop[Stop pagination]
Fetch --> Grid
Reviews (2): Last reviewed commit: "fix(tables): keep an emptied view termin..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9861887. Configure here.
Summary
nextCursorinstead of an advisory count, so a byte-cut short page can't be misreadThe bug
hooks/queries/tables.tssendsincludeTotal: param === 0as a boolean.requestJsonparses the query through the contract on the client before building the URL (lib/api/client/request.ts—parseOptionalSchemaruns beforeappendQuery), and the contract's preprocess only recognised the string'true':So
totalCountcame backnullon every table, every load.hasMoreTableRowstreats a null total as "more may exist", sohasNextPagewas permanentlytrue:handleAppendRowdrains before inserting → every New Row click issued a wastedGET /rowsreturning 0 rows, which setsisFetchingNextPageand renders the spinner rowrowTotalwas permanentlynull, so "select all" silently fell back to the unfilteredrowCount— wrong under a filterThis was latent from the start and became visible when #5351 (2026-07-03) removed the
rows.length < pageSizeguard that had been masking it. Fixed by reusing the existingbooleanQueryFlagSchemaprimitive, which already accepts both forms.I audited the other seven contracts using a string-only
=== 'true'coercion: all of them usez.string()orz.enum(['true','false']), which reject a boolean loudly rather than silently inverting it. This was the only silent instance, so nothing else changed.Terminating on the cursor
totalCountis advisory, and since #6582 a page can legitimately be shorter thanlimit(byte cut). The response already carriednextCursor— non-null exactly when the drain proved an unreturned witness row — but the client discarded it. Now threaded through and preferred, with the count rules kept as a fallback for pages cached before this change. That also closes #6582's exposure: its own removed comment warned that a short page is only safe for a client terminating onnextCursor, and this grid was the pre-existing pager it described.One round-trip for the read guards
withReadGuardsissuedSET LOCAL statement_timeoutandSET LOCAL lock_timeoutas separate awaited statements (plus a third forenable_seqscan). The driver runsprepare: false, so each was its own serial round-trip, on every page, count, and drain batch. They now go in a singleset_config(...)SELECT — same transaction-local scope.set_configrather than semicolon-joinedSET LOCALbecause the driver uses the extended protocol, which rejects multiple commands per message.enable_seqscanis still only ever set tooff, never explicitlyon, so the unflagged path keeps the server default.Not in this PR
rowsRootand refetches every loaded page — defeating the optimistic work inuseCreateTableRow/useUpdateTableRowabout a second later.lib/table/events.tsdocuments this and names the fix. It's cleanly doable via the existingRequestContextAsyncLocalStorage with no changes to the ~49 signal call sites, but it touches shared request infrastructure for a tables-specific benefit, so it belongs in its own PR.firstBatchCap. The first drain batch is capped at 51 rows regardless of the requested 1000, so every page costs two transactions. The cap is load-bearing — it bounds worst-case memory at 4x the byte budget assuming max-size rows, and the DB returns every asked-for row before JS can byte-cut, so raising it to the caller's limit risks materializing ~400MB. Needs a server-side byte-bounded fetch, not a constant change.Type of Change
Testing
Unit tests for both fixes, each verified by reverting the fix and confirming the matching test goes red:
expected false to be truewithout the coercion fixnextCursorbranchNot yet verified in a browser. The check that would confirm it end-to-end: on a ~10-row table, exactly one
GET /rowson load (currently two),totalCount: 10rather thannull, and New Row issuing only the POST with no spinner.Checklist
Audit follow-up
Three adversarial audits were run against this diff, one per fix. Two found real problems, fixed in the second commit.
Behavior changes worth knowing (not pure perf)
Select-all now respects the active filter.
rowTotalhas been permanentlynullfor months, soselectAllTotalRefsilently fell back to the table's unfilteredrowCount. Everything downstream — bulk delete'sestimatedCount, run scope, the selected-count label, clipboard "complete:" — was therefore mis-scoped in a filtered view. RestoringtotalCountfixes that. It is a correctness fix, not a regression, but it does change what those numbers say.Restoring
COUNT(*)costs latency on large filtered/sorted views. The count runs in parallel with the page drain, so the cost ismax()not additive, but a filtered count goes throughwithSeqscanOff— on a 1M-row table the planner comment measures filtered count ~1.0s vs sorted page ~0.76s, so page 0 there goes roughly 0.76s → 1.0s. Accepted deliberately: the alternative is continuing to mis-scope select-all. This fix is a correctness restoration that costs latency; it is thenextCursorchange that removes the wasted page fetch.?includeTotal=yesand repeated keys now 400 on public v1 instead of silently meaningfalse. Kept rather than softened, becauselimitandoffsetin the same schema have always thrown on garbage — the silent swallow was the inconsistency.'1','TRUE', and' true 'now parse as true (previously false). v2 is unaffected —includeTotalis not on its wire.Bugs the audits caught in this diff
useDeleteTableRowsAsyncrelied on row-count arithmetic to suppresshasNextPagewhile the background job runs. Preferring the cursor bypassed that, so "select all, then deselect a few" on a partially-drained view would refetch the very rows being deleted. Fixed by pinningnextCursor: nullon the emptied pages, with a test.hasFilterread onlyuserClause, ignoringdeleteMask— so during a running delete job the count took the non-seqscanOffplan despite JSONB predicates, bounded only by the 15s timeout. Pre-existing, but unreachable from the grid until page-0 counts came back. NowBoolean(userClause || deleteMask).MAX_QUERY_BATCHESandMAX_QUERY_LIMITare exactly tight (both 1000). The drain's bound is only unreachable because the limit cut fires first; the count-based net that used to paper over it is gone now that clients terminate on the cursor. Documented as an invariant — raise both together.Verified, not assumed
set_configchange was run against a real PostgreSQL 17 through this repo's exact driver stack: identical GUC normalization toSET LOCAL, identical savepoint-rollback behavior, bound parameters accepted, and an invalid value aborts the transaction before the read runs — so there is no path where a read proceeds unguarded.nextCursorsurvives every optimistic update.TableRowsResponsedeclares it required, so a future page literal that omits it is a compile error.hasMoreis only ever set with a fetched-but-unreturned witness row, for keyset, offset, filtered, and byte-cut pages alike.