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

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) {

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

@@ -51,6 +51,35 @@ 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 for pages cached before the cursor was threaded through', () => {
+      expect(hasMoreTableRows([makePage(36, 100)])).toBe(true)
+      expect(hasMoreTableRows([makePage(3, 3)])).toBe(false)
+    })
+  })
 })
 
 describe('getNextTableRowsPageParam', () => {

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

@@ -9,6 +9,11 @@ export type TableRowsPageParam = number | TableRowsCursor
 interface TableRowsPageLike {
   rows: ReadonlyArray<{ id: string; orderKey?: string }>
   totalCount: number | null
+  /**
+   * Optional because pages cached before this field was threaded through predate it; those fall
+   * back to the count rules below.
+   */
+  nextCursor?: string | null
 }
 
 /** Rows loaded across all fetched pages. */
@@ -17,18 +22,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,42 @@
  * @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`, page 0 came back with
+ * `totalCount: null`, and `hasMoreTableRows` — which treats a null total as "more may exist" —
+ * reported `hasNextPage` forever. Every table then paid a wasted extra page fetch on mount and
+ * before every row insert.
+ */
+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,17 @@ 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`.
+   */
   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,32 @@ 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 `trx.execute` is its own serial round-trip (the
+ * driver runs `prepare: false`), 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 — but it is a function call, so several fit in a single `SELECT`. Semicolon-
+ * joining `SET LOCAL` statements would not work here: the driver sends this over the extended
+ * protocol, which rejects multiple commands in one message.
+ */
+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 +52,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)
   })
 }