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

* fix(tables): stop every table paginating forever on a null totalCount

* fix(tables): keep an emptied view terminated, and count masked reads off the seq scan

Author: waleedlatif1

apps/sim/hooks/queries/tables.ts

@@ -140,7 +140,7 @@ type TableRowsParams = Omit<TableRowsQueryInput, 'filter' | 'sort'> &
 
 export type TableRowsResponse = Pick<
   ContractJsonResponse<typeof listTableRowsContract>['data'],
-  'rows' | 'totalCount'
+  'rows' | 'totalCount' | 'nextCursor'
 >
 
 interface RowMutationContext {
@@ -195,8 +195,13 @@ async function fetchTableRows({
     },
     signal,
   })
-  const { rows, totalCount } = response.data
-  return { rows, totalCount }
+  const { rows, totalCount, nextCursor } = response.data
+  /**
+   * `nextCursor` is kept because it is the only authoritative end-of-table signal: the server
+   * sets it exactly when the drain proved an unreturned witness row, so it covers a page cut by
+   * the byte budget as well as one cut by `limit`. See {@link hasMoreTableRows}.
+   */
+  return { rows, totalCount, nextCursor }
 }
 
 function invalidateRowCount(queryClient: ReturnType<typeof useQueryClient>, tableId: string) {
@@ -1295,6 +1300,14 @@ export function useDeleteTableRowsAsync({ workspaceId, tableId }: RowMutationCon
                   ...page,
                   rows: page.rows.filter((r) => keep.has(r.id)),
                   ...(page.totalCount != null ? { totalCount: keep.size } : {}),
+                  /**
+                   * The view is being emptied on purpose, so it has no next page — stated
+                   * explicitly because the server's cursor would otherwise say otherwise and
+                   * scrolling would pull back the very rows the job is deleting. Only the
+                   * row-count arithmetic used to carry this, which {@link hasMoreTableRows}
+                   * no longer consults once a cursor is present.
+                   */
+                  nextCursor: null,
                 })),
               }
             : old

apps/sim/hooks/queries/utils/table-rows-pagination.test.ts

@@ -51,6 +51,49 @@ describe('hasMoreTableRows', () => {
   it('returns false when a stale-low count is already exceeded', () => {
     expect(hasMoreTableRows([makePage(10, 5)])).toBe(false)
   })
+
+  /**
+   * The server sets `nextCursor` exactly when the drain proved an unreturned witness row, so it
+   * answers correctly for a page cut by the byte budget — where both page fullness and the count
+   * mislead. It therefore wins over the count rules whenever it is present.
+   */
+  describe('nextCursor', () => {
+    it('ends the drain on a null cursor even when the count claims more rows', () => {
+      expect(hasMoreTableRows([{ ...makePage(36, 100), nextCursor: null }])).toBe(false)
+    })
+
+    it('continues on a non-null cursor even when the count is already covered', () => {
+      // A byte-cut page: fewer rows than asked for, and the advisory count disagrees.
+      expect(hasMoreTableRows([{ ...makePage(3, 3), nextCursor: 'c1' }])).toBe(true)
+    })
+
+    it('reads the cursor from the last page, not page 0', () => {
+      const pages = [
+        { ...makePage(1000, null), nextCursor: 'c1' },
+        { ...makePage(12, null, 1000), nextCursor: null },
+      ]
+      expect(hasMoreTableRows(pages)).toBe(false)
+    })
+
+    it('falls back to the count rules when a page carries no cursor', () => {
+      expect(hasMoreTableRows([makePage(36, 100)])).toBe(true)
+      expect(hasMoreTableRows([makePage(3, 3)])).toBe(false)
+    })
+
+    /**
+     * The async "select all" delete strips rows from the active view and pins `nextCursor: null`
+     * so scrolling cannot pull back the rows the background job is still deleting. Deselecting a
+     * few leaves kept rows on the last page, so the row-count arithmetic that used to suppress
+     * `hasNextPage` no longer fires — only the pinned cursor does.
+     */
+    it('stays terminated for a partially-emptied view whose pages pin a null cursor', () => {
+      const pages = [
+        { ...makePage(2, 2), nextCursor: null },
+        { ...makePage(1, null, 2), nextCursor: null },
+      ]
+      expect(hasMoreTableRows(pages)).toBe(false)
+    })
+  })
 })
 
 describe('getNextTableRowsPageParam', () => {

apps/sim/hooks/queries/utils/table-rows-pagination.ts

@@ -9,6 +9,13 @@ export type TableRowsPageParam = number | TableRowsCursor
 interface TableRowsPageLike {
   rows: ReadonlyArray<{ id: string; orderKey?: string }>
   totalCount: number | null
+  /**
+   * Optional only so this loose page shape stays usable by callers that do not have a server
+   * response to hand (tests, and the optimistic mappings). On the wire it is required — the
+   * contract declares it non-optional and `requestJson` validates the response — so a real page
+   * always carries it and the count fallback below is defensive, not a live path.
+   */
+  nextCursor?: string | null
 }
 
 /** Rows loaded across all fetched pages. */
@@ -17,18 +24,23 @@ export function countLoadedTableRows(pages: readonly TableRowsPageLike[]): numbe
 }
 
 /**
- * Whether more rows may exist past the fetched pages. A page is terminal only when it is
- * empty or when page 0's `COUNT(*)` is already covered — never when it is merely shorter
- * than the requested page size, so a short server page can never be misread as end-of-table.
+ * Whether more rows may exist past the fetched pages.
  *
- * `totalCount` is advisory (computed in a separate transaction from the page read). A
- * stale-high count self-corrects via the empty-page rule at the cost of one extra request;
- * a stale-low count (rows deleted after page 0's COUNT) stops the drain early — accepted,
- * since the view is already stale and the run-stream/interval invalidations refetch it.
+ * `nextCursor` is the authoritative answer and is preferred whenever the server sent one: it is
+ * non-null exactly when the drain proved an unreturned witness row, so it is correct for a page
+ * cut by the byte budget as well as one cut by `limit`. Page fullness cannot answer this — a
+ * byte-cut page is legitimately shorter than the requested size.
+ *
+ * The count rules remain as a fallback for pages cached before `nextCursor` was threaded through.
+ * They are weaker: `totalCount` is advisory (computed in a separate transaction from the page
+ * read), so a stale-high count self-corrects via the empty-page rule at the cost of one extra
+ * request, and a stale-low count stops the drain early. A null `totalCount` is read as "unknown,
+ * assume more" — which is why the `includeTotal` coercion bug made every table page forever.
  */
 export function hasMoreTableRows(pages: readonly TableRowsPageLike[]): boolean {
   const lastPage = pages[pages.length - 1]
   if (!lastPage || lastPage.rows.length === 0) return false
+  if (lastPage.nextCursor !== undefined) return lastPage.nextCursor !== null
   const totalCount = pages[0].totalCount
   return totalCount == null || countLoadedTableRows(pages) < totalCount
 }

apps/sim/lib/api/contracts/tables.test.ts

@@ -2,7 +2,46 @@
  * @vitest-environment node
  */
 import { describe, expect, it } from 'vitest'
-import { tableEventStreamQuerySchema } from '@/lib/api/contracts/tables'
+import { tableEventStreamQuerySchema, tableRowsQuerySchema } from '@/lib/api/contracts/tables'
+
+/**
+ * `requestJson` parses the query through this schema on the CLIENT before building the URL, so
+ * these values arrive as the caller's real types, not as URL strings. A string-only coercion
+ * therefore read the grid's `includeTotal: param === 0` boolean as `false`, and page 0 came back
+ * with `totalCount: null` on every table.
+ *
+ * What that broke is the **filtered** total: `rowTotal` was permanently null, so select-all and
+ * everything downstream of it (bulk delete, run scope, the selected-count label) silently fell
+ * back to the table's UNFILTERED `rowCount`. It also left `hasMoreTableRows` reading a null total
+ * as "more may exist" — though that half is now answered by `nextCursor` instead, so this schema
+ * is not what removes the wasted page fetch.
+ */
+describe('tableRowsQuerySchema includeTotal', () => {
+  it('accepts a real boolean, which is what the client passes', () => {
+    expect(
+      tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: true }).includeTotal
+    ).toBe(true)
+    expect(
+      tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: false }).includeTotal
+    ).toBe(false)
+  })
+
+  it('still accepts the URL strings a direct API caller sends', () => {
+    expect(
+      tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: 'true' }).includeTotal
+    ).toBe(true)
+    expect(
+      tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: 'false' }).includeTotal
+    ).toBe(false)
+  })
+
+  it('defaults to true when absent or empty, so a bare request still gets its count', () => {
+    expect(tableRowsQuerySchema.parse({ workspaceId: 'ws-1' }).includeTotal).toBe(true)
+    expect(tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: '' }).includeTotal).toBe(
+      true
+    )
+  })
+})
 
 describe('tableEventStreamQuerySchema', () => {
   it('parses an explicit cursor', () => {

apps/sim/lib/api/contracts/tables.ts

@@ -1,6 +1,7 @@
 import { isRecordLike } from '@sim/utils/object'
 import { z } from 'zod'
 import {
+  booleanQueryFlagSchema,
   folderIdSchema,
   privateSecretProvenanceBundleSchema,
   requiredFieldSchema,
@@ -800,11 +801,21 @@ export const tableRowsQueryBaseSchema = z.object({
         .optional()
     )
     .default(0),
+  /**
+   * Absent, null, and empty all fall through to the `true` default, so a bare request still
+   * gets its count. Everything else goes to {@link booleanQueryFlagSchema}, which accepts a real
+   * boolean as well as the URL strings — `requestJson` parses this schema on the CLIENT before
+   * building the URL, so the value arrives as the caller's own type, and a string-only coercion
+   * silently read the grid's `includeTotal: param === 0` as `false` — leaving `totalCount` null on
+   * every table, and select-all falling back to the unfiltered row count.
+   *
+   * Unparseable values now reject rather than resolving to `false`, matching `limit` and `offset`
+   * in this same schema, which have always thrown on garbage.
+   */
   includeTotal: z
     .preprocess(
-      (value) =>
-        value === null || value === undefined || value === '' ? undefined : value === 'true',
-      z.boolean().optional()
+      (value) => (value === null || value === undefined || value === '' ? undefined : value),
+      booleanQueryFlagSchema.optional()
     )
     .default(true),
 })

apps/sim/lib/table/planner.ts

@@ -15,14 +15,36 @@ export type DbTransaction = Parameters<Parameters<typeof db.transaction>[0]>[0]
 const READ_STATEMENT_TIMEOUT_MS = 15_000
 const READ_LOCK_TIMEOUT_MS = 3_000
 
-async function setReadTimeouts(trx: DbTransaction): Promise<void> {
-  await trx.execute(sql.raw(`SET LOCAL statement_timeout = '${READ_STATEMENT_TIMEOUT_MS}ms'`))
-  await trx.execute(sql.raw(`SET LOCAL lock_timeout = '${READ_LOCK_TIMEOUT_MS}ms'`))
+/**
+ * Applies every guard in ONE round-trip. Each awaited `trx.execute` is its own round-trip, and
+ * every user-table read opens a transaction, so issuing these separately cost 2–3 round-trips on
+ * every page, count, and drain batch.
+ *
+ * `set_config(name, value, is_local => true)` is exactly `SET LOCAL` — transaction-scoped, dying
+ * with the commit, and reverting the same way on a savepoint rollback — but it is a function call,
+ * so several fit in one `SELECT`. It also takes the values as bound parameters, which `SET LOCAL`
+ * cannot. That is the reason they must be one statement rather than semicolon-joined: a bound
+ * parameter forces the extended protocol, which rejects multiple commands per message.
+ *
+ * The guards are the first statement in the transaction, so an invalid value aborts it before
+ * `fn(trx)` can run — there is no path where a read proceeds unguarded.
+ */
+async function setReadGuards(trx: DbTransaction, seqscanOff: boolean): Promise<void> {
+  /**
+   * Only ever set to `off`, never explicitly to `on` — the unflagged path must leave whatever
+   * the server default is, exactly as the separate `SET LOCAL enable_seqscan = off` did.
+   */
+  const seqscan = seqscanOff ? sql`, set_config('enable_seqscan', 'off', true)` : sql``
+  await trx.execute(sql`
+    select
+      set_config('statement_timeout', ${`${READ_STATEMENT_TIMEOUT_MS}ms`}, true),
+      set_config('lock_timeout', ${`${READ_LOCK_TIMEOUT_MS}ms`}, true)${seqscan}
+  `)
 }
 
 /**
  * Runs a user-table read inside a transaction that always caps `statement_timeout`
- * / `lock_timeout` (see {@link setReadTimeouts}). Pass `seqscanOff` for queries
+ * / `lock_timeout` (see {@link setReadGuards}). Pass `seqscanOff` for queries
  * with no tenant-bounded index plan — custom column sorts and filtered counts —
  * where the planner otherwise seq-scans the whole shared `user_table_rows`
  * relation (every tenant's rows); see {@link withSeqscanOff} for the measured
@@ -34,8 +56,7 @@ export async function withReadGuards<T>(
   opts?: { seqscanOff?: boolean }
 ): Promise<T> {
   return db.transaction(async (trx) => {
-    await setReadTimeouts(trx)
-    if (opts?.seqscanOff) await trx.execute(sql`SET LOCAL enable_seqscan = off`)
+    await setReadGuards(trx, opts?.seqscanOff ?? false)
     return fn(trx)
   })
 }

apps/sim/lib/table/rows/service.ts

@@ -1152,7 +1152,13 @@ export async function queryRows(
   // unfiltered count already plans an index-only scan on the table_id prefix.
   // The count uses the full-view WHERE (no cursor seek): totals cover the whole
   // view, not the remaining pages.
-  const hasFilter = Boolean(userClause)
+  /**
+   * The delete mask counts as a filter: it injects JSONB predicates into `baseConditions`, which
+   * is exactly the plan shape `countRowsTenantBounded` exists to keep off a seq scan of the shared
+   * relation. Reading only `userClause` sent a masked-but-unfiltered count down the plain branch,
+   * bounded only by the statement timeout.
+   */
+  const hasFilter = Boolean(userClause || deleteMask)
   const countPromise = includeTotal
     ? hasFilter
       ? countRowsTenantBounded(whereClause)
@@ -1264,7 +1270,15 @@ interface BoundedFetchResult {
   anchorOffset: number
 }
 
-/** Belt-and-braces bound on drain iterations; unreachable in practice. */
+/**
+ * Belt-and-braces bound on drain iterations.
+ *
+ * Unreachable only because every iteration either consumes at least one row or cuts, and a bounded
+ * page's `limit` is capped at {@link TABLE_LIMITS.MAX_QUERY_LIMIT} — so the limit cut always fires
+ * first. That makes the two constants exactly tight: raising `MAX_QUERY_LIMIT` above this bound
+ * would let the loop exit with rows still unread and `hasMore: false`, which clients now trust as
+ * end-of-table (they terminate on `nextCursor`, which this decides). Raise both together.
+ */
 const MAX_QUERY_BATCHES = 1000
 
 /**