Skip to content

fix(tables): stop every table paginating forever on a null totalCount - #6694

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/tables-new-row-optimistic
Aug 14, 2026
Merged

fix(tables): stop every table paginating forever on a null totalCount#6694
waleedlatif1 merged 2 commits into
stagingfrom
fix/tables-new-row-optimistic

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fix every table paginating forever, which cost an extra row-page fetch on load and a wasted one before every row insert (the "New Row" spinner)
  • Terminate row paging on the server's nextCursor instead of an advisory count, so a byte-cut short page can't be misread
  • Collapse the per-read transaction guards into one round-trip

The bug

hooks/queries/tables.ts sends includeTotal: param === 0 as a boolean. requestJson parses the query through the contract on the client before building the URL (lib/api/client/request.tsparseOptionalSchema runs before appendQuery), and the contract's preprocess only recognised the string 'true':

boolean true  -> false      <-- page 0 asked for no COUNT(*)
string 'true' -> true
undefined     -> true

So totalCount came back null on every table, every load. hasMoreTableRows treats a null total as "more may exist", so hasNextPage was permanently true:

  • handleAppendRow drains before inserting → every New Row click issued a wasted GET /rows returning 0 rows, which sets isFetchingNextPage and renders the spinner row
  • the scroll-prefetch effect fired an extra request on mount
  • rowTotal was permanently null, so "select all" silently fell back to the unfiltered rowCount — wrong under a filter

This was latent from the start and became visible when #5351 (2026-07-03) removed the rows.length < pageSize guard that had been masking it. Fixed by reusing the existing booleanQueryFlagSchema primitive, which already accepts both forms.

I audited the other seven contracts using a string-only === 'true' coercion: all of them use z.string() or z.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

totalCount is advisory, and since #6582 a page can legitimately be shorter than limit (byte cut). The response already carried nextCursor — 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 on nextCursor, and this grid was the pre-existing pager it described.

One round-trip for the read guards

withReadGuards issued SET LOCAL statement_timeout and SET LOCAL lock_timeout as separate awaited statements (plus a third for enable_seqscan). The driver runs prepare: false, so each was its own serial round-trip, on every page, count, and drain batch. They now go in a single set_config(...) SELECT — same transaction-local scope. set_config rather than semicolon-joined SET LOCAL because the driver uses the extended protocol, which rejects multiple commands per message. enable_seqscan is still only ever set to off, never explicitly on, so the unflagged path keeps the server default.

Not in this PR

  • The SSE self-echo. The collaboration signal added in feat(realtime): shared room spine + live Files/Tables collaboration + Yjs document editing #5991 is echoed to the client that made the write, which invalidates rowsRoot and refetches every loaded page — defeating the optimistic work in useCreateTableRow/useUpdateTableRow about a second later. lib/table/events.ts documents this and names the fix. It's cleanly doable via the existing RequestContext AsyncLocalStorage 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

  • Bug fix

Testing

Unit tests for both fixes, each verified by reverting the fix and confirming the matching test goes red:

  • the contract test fails with expected false to be true without the coercion fix
  • the three cursor tests fail without the nextCursor branch

Not yet verified in a browser. The check that would confirm it end-to-end: on a ~10-row table, exactly one GET /rows on load (currently two), totalCount: 10 rather than null, and New Row issuing only the POST with no spinner.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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. rowTotal has been permanently null for months, so selectAllTotalRef silently fell back to the table's unfiltered rowCount. Everything downstream — bulk delete's estimatedCount, run scope, the selected-count label, clipboard "complete:" — was therefore mis-scoped in a filtered view. Restoring totalCount fixes 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 is max() not additive, but a filtered count goes through withSeqscanOff — 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 the nextCursor change that removes the wasted page fetch.

?includeTotal=yes and repeated keys now 400 on public v1 instead of silently meaning false. Kept rather than softened, because limit and offset in 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 — includeTotal is not on its wire.

Bugs the audits caught in this diff

  • An emptied view could pull back rows mid-delete. useDeleteTableRowsAsync relied on row-count arithmetic to suppress hasNextPage while 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 pinning nextCursor: null on the emptied pages, with a test.
  • A masked count could seq-scan the shared relation. hasFilter read only userClause, ignoring deleteMask — so during a running delete job the count took the non-seqscanOff plan despite JSONB predicates, bounded only by the 15s timeout. Pre-existing, but unreachable from the grid until page-0 counts came back. Now Boolean(userClause || deleteMask).
  • MAX_QUERY_BATCHES and MAX_QUERY_LIMIT are 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

  • The set_config change was run against a real PostgreSQL 17 through this repo's exact driver stack: identical GUC normalization to SET 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.
  • All eight rows-cache mutation sites spread the original page, so nextCursor survives every optimistic update. TableRowsResponse declares it required, so a future page literal that omits it is a compile error.
  • No premature-termination path found: hasMore is only ever set with a fetched-but-unreturned witness row, for keyset, offset, filtered, and byte-cut pages alike.

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 14, 2026 3:55am

Request Review

@cursor

cursor Bot commented Aug 14, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes core table pagination and filtered totals (select-all/bulk delete scope) plus DB read guard wiring; well-tested but affects hot paths on every table load and scroll.

Overview
Fixes infinite table row pagination and related select-all/count bugs by restoring real totalCount on page 0 and treating the server's nextCursor as the authoritative “more rows” signal.

includeTotal on list rows now uses booleanQueryFlagSchema, so the grid's client-side includeTotal: param === 0 boolean is parsed correctly (previously only the string 'true' worked, so every load sent “no count” and totalCount was always null). That mis-scoped filtered select-all, bulk delete estimates, and kept hasMoreTableRows assuming more rows forever.

Pagination threads nextCursor through the rows query response and hasMoreTableRows prefers it over advisory totalCount, including for byte-budget-short pages. Async select-all delete optimistically pins nextCursor: null on emptied pages so scroll prefetch cannot reload rows the background job is deleting.

Server-side: filtered row counts treat an active delete mask as a filter (tenant-bounded count path); read transaction guards (statement_timeout, lock_timeout, optional enable_seqscan) are applied in a single set_config round-trip per read transaction.

Reviewed by Cursor Bugbot for commit 9861887. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR fixes table pagination and filtered totals by accepting boolean query flags, retaining the server’s authoritative cursor, and consolidating transaction-local read guards.

  • Threads nextCursor through the frontend row-page cache and uses it to determine pagination completion.
  • Restores page-zero totals by correctly parsing boolean includeTotal values.
  • Prevents asynchronous deletion views from fetching rows being deleted.
  • Applies read timeout and planner guards in one database round trip.
  • Routes delete-masked counts through the tenant-bounded planning path.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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
Loading

Reviews (2): Last reviewed commit: "fix(tables): keep an emptied view termin..." | Re-trigger Greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@waleedlatif1
waleedlatif1 merged commit b2b6e55 into staging Aug 14, 2026
23 of 25 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/tables-new-row-optimistic branch August 14, 2026 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant