improvement(tables): stop re-reading values the request already holds

apps/sim/app/api/table/capability-gate.test.ts

@@ -20,14 +20,26 @@ import {
 import { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockGetTableById, mockGetUserEntityPermissions, mockAddTableColumn, mockListTableViews } =
+const { mockGetTableById, mockCheckWorkspaceAccess, mockAddTableColumn, mockListTableViews } =
   vi.hoisted(() => ({
     mockGetTableById: vi.fn(),
-    mockGetUserEntityPermissions: vi.fn(),
+    mockCheckWorkspaceAccess: vi.fn(),
     mockAddTableColumn: vi.fn(),
     mockListTableViews: vi.fn(),
   }))
 
+/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */
+function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') {
+  return {
+    exists: true,
+    hasAccess: permission !== null,
+    canWrite: permission === 'admin' || permission === 'write',
+    canAdmin: permission === 'admin',
+    workspace: { id: 'ws-1', organizationId },
+    permission,
+  }
+}
+
 vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock)
 
 vi.mock('@/lib/table', () => ({
@@ -44,7 +56,7 @@ vi.mock('@/lib/table/events', () => ({ signalTableSchemaChanged: vi.fn() }))
 vi.mock('@/lib/table/orchestration', () => ({ performUpdateTableColumn: vi.fn() }))
 vi.mock('@/lib/table/wire', () => ({ normalizeColumn: (column: unknown) => column }))
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
-  getUserEntityPermissions: mockGetUserEntityPermissions,
+  checkWorkspaceAccess: mockCheckWorkspaceAccess,
 }))
 vi.mock('@/lib/workspaces/utils', () => ({ getWorkspaceOrganizationId: vi.fn() }))
 
@@ -96,11 +108,42 @@ describe('tables.use gate on the raw /api/table routes', () => {
       authType: 'session',
     })
     mockGetTableById.mockResolvedValue(TABLE)
-    mockGetUserEntityPermissions.mockResolvedValue('admin')
+    mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin'))
     mockAddTableColumn.mockResolvedValue({ schema: { columns: [{ name: 'expires_at' }] } })
     mockListTableViews.mockResolvedValue([])
   })
 
+  /**
+   * The capability resolver looks the workspace up itself when the organization is omitted, so a
+   * call site that already access-checked the workspace and drops the id pays a second read of a
+   * value it is holding — once on every raw table route. Asserted on the resolver rather than on
+   * a query count because that is where the omission would show.
+   */
+  it('hands the capability resolver the organization it just loaded, not undefined', async () => {
+    mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin', 'org-42'))
+
+    await listViews()
+
+    expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).toHaveBeenCalledWith(
+      expect.any(String),
+      expect.any(String),
+      'org-42'
+    )
+  })
+
+  /** A personal workspace has no organization; `null` is the answer, and still not a lookup. */
+  it('passes null for a workspace that belongs to no organization', async () => {
+    mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin', null))
+
+    await listViews()
+
+    expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).toHaveBeenCalledWith(
+      expect.any(String),
+      expect.any(String),
+      null
+    )
+  })
+
   describe('when the group withholds Tables', () => {
     beforeEach(() => {
       permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({
@@ -131,7 +174,7 @@ describe('tables.use gate on the raw /api/table routes', () => {
     })
 
     it('still conceals a table the caller cannot reach, rather than naming the capability', async () => {
-      mockGetUserEntityPermissions.mockResolvedValue(null)
+      mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess(null))
 
       const response = await listViews()
 

apps/sim/app/api/table/utils.ts

@@ -23,7 +23,7 @@ import { TableLockedError } from '@/lib/table/mutation-locks'
 import { isTablePredicate } from '@/lib/table/query-builder/converters'
 import { validateStoragePredicate } from '@/lib/table/query-builder/validate'
 import type { TableLockKind } from '@/lib/table/types'
-import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
+import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils'
 import { getWorkspaceOrganizationId } from '@/lib/workspaces/utils'
 
 /**
@@ -338,12 +338,14 @@ export async function checkAccess(
     return { ok: false, status: 404 }
   }
 
-  const permission = await getUserEntityPermissions(
-    roleSubjectUserId(principal),
-    'workspace',
-    table.workspaceId
-  )
-  if (!permissionSatisfies(permission, level)) {
+  /**
+   * Resolved through {@link checkWorkspaceAccess} rather than `getUserEntityPermissions`, which
+   * delegates to it and returns the permission alone. Same single resolution, but it also hands
+   * back the workspace this check just loaded — and with it the owning organization the
+   * capability gate below would otherwise look up for itself.
+   */
+  const access = await checkWorkspaceAccess(table.workspaceId, roleSubjectUserId(principal))
+  if (!permissionSatisfies(access.permission, level)) {
     return { ok: false, status: 403 }
   }
 
@@ -352,7 +354,18 @@ export async function checkAccess(
   if (
     governedUserId &&
     table.workspaceId &&
-    (await isWorkspaceCapabilityWithheld(governedUserId, table.workspaceId, 'tables.use'))
+    /**
+     * The organization is passed, not re-derived: omitting it makes the resolver load this very
+     * workspace a second time (see `getUserPermissionConfig`), which is one extra round trip on
+     * every raw table route. `access.workspace` is non-null on this line — a missing workspace
+     * resolves to a null permission, which the gate above already refused.
+     */
+    (await isWorkspaceCapabilityWithheld(
+      governedUserId,
+      table.workspaceId,
+      'tables.use',
+      access.workspace?.organizationId ?? null
+    ))
   ) {
     return { ok: false, status: 403, capability: 'tables.use' }
   }

apps/sim/app/api/v1/tables/[tableId]/route.test.ts

@@ -17,18 +17,30 @@ const {
   mockCheckRateLimit,
   mockCheckWorkspaceScope,
   mockGetTableById,
-  mockGetUserEntityPermissions,
+  mockCheckWorkspaceAccess,
   mockPerformDeleteTable,
   mockResolveWorkspaceRequestActor,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockCheckWorkspaceScope: vi.fn(),
   mockGetTableById: vi.fn(),
-  mockGetUserEntityPermissions: vi.fn(),
+  mockCheckWorkspaceAccess: vi.fn(),
   mockPerformDeleteTable: vi.fn(),
   mockResolveWorkspaceRequestActor: vi.fn(),
 }))
 
+/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */
+function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') {
+  return {
+    exists: true,
+    hasAccess: permission !== null,
+    canWrite: permission === 'admin' || permission === 'write',
+    canAdmin: permission === 'admin',
+    workspace: { id: 'ws-1', organizationId },
+    permission,
+  }
+}
+
 vi.mock('@/app/api/v1/middleware', () => ({
   checkRateLimit: mockCheckRateLimit,
   checkWorkspaceScope: mockCheckWorkspaceScope,
@@ -69,7 +81,10 @@ vi.mock('@/lib/table', () => ({
 }))
 
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
-  getUserEntityPermissions: mockGetUserEntityPermissions,
+  checkWorkspaceAccess: mockCheckWorkspaceAccess,
+  /** The v1 middleware reads the permission alone; `checkAccess` reads the whole access. */
+  getUserEntityPermissions: async (...args: unknown[]) =>
+    (await mockCheckWorkspaceAccess(...args)).permission,
 }))
 
 vi.mock('@/lib/workspaces/utils', () => ({
@@ -117,7 +132,7 @@ describe('DELETE /api/v1/tables/[tableId] — orchestration failure projection',
       name: 'Table',
       workspaceId: WORKSPACE_ID,
     })
-    mockGetUserEntityPermissions.mockResolvedValue('admin')
+    mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin'))
   })
 
   /**

apps/sim/app/api/v1/tables/capability-gate.test.ts

@@ -29,20 +29,35 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
   mockAuthenticateV1Request,
-  mockGetUserEntityPermissions,
+  mockCheckWorkspaceAccess,
   mockGetWorkspaceBillingSettings,
   mockGetTableById,
 } = vi.hoisted(() => ({
   mockAuthenticateV1Request: vi.fn(),
-  mockGetUserEntityPermissions: vi.fn(),
+  mockCheckWorkspaceAccess: vi.fn(),
   mockGetWorkspaceBillingSettings: vi.fn(),
   mockGetTableById: vi.fn(),
 }))
 
+/** The shape `checkAccess` reads: the viewer's permission plus the workspace it just loaded. */
+function workspaceAccess(permission: string | null, organizationId: string | null = 'org-1') {
+  return {
+    exists: true,
+    hasAccess: permission !== null,
+    canWrite: permission === 'admin' || permission === 'write',
+    canAdmin: permission === 'admin',
+    workspace: { id: 'ws-1', organizationId },
+    permission,
+  }
+}
+
 vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock)
 vi.mock('@/app/api/v1/auth', () => ({ authenticateV1Request: mockAuthenticateV1Request }))
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
-  getUserEntityPermissions: mockGetUserEntityPermissions,
+  checkWorkspaceAccess: mockCheckWorkspaceAccess,
+  /** The v1 middleware reads the permission alone; `checkAccess` reads the whole access. */
+  getUserEntityPermissions: async (...args: unknown[]) =>
+    (await mockCheckWorkspaceAccess(...args)).permission,
 }))
 vi.mock('@/lib/workspaces/utils', () => ({
   getWorkspaceBillingSettings: mockGetWorkspaceBillingSettings,
@@ -103,7 +118,7 @@ beforeEach(() => {
   vi.clearAllMocks()
   resetPermissionGroupScopeMock()
   mockAuthenticateV1Request.mockResolvedValue(v1PersonalKeyCredential(MEMBER_ID))
-  mockGetUserEntityPermissions.mockResolvedValue('admin')
+  mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess('admin'))
   mockGetWorkspaceBillingSettings.mockResolvedValue({ allowPersonalApiKeys: true })
   mockGetTableById.mockResolvedValue(TABLE)
 })
@@ -147,7 +162,7 @@ describe('tables.use gate on /api/v1/tables/[tableId]', () => {
   })
 
   it('still refuses either key kind on role, before naming the capability', async () => {
-    mockGetUserEntityPermissions.mockResolvedValue(null)
+    mockCheckWorkspaceAccess.mockResolvedValue(workspaceAccess(null))
     governedBy({ hideTablesTab: true })
 
     const response = await readTable()

apps/sim/lib/table/rows/__tests__/ordering-anchor.test.ts

@@ -5,7 +5,7 @@ import { describe, expect, it } from 'vitest'
 import { OrchestrationError } from '@/lib/core/orchestration/types'
 import type { DbTransaction } from '@/lib/table/planner'
 import { TableRowNotFoundError } from '@/lib/table/rows/errors'
-import { resolveInsertByNeighbor } from '@/lib/table/rows/ordering'
+import { appendAnchors, resolveInsertByNeighbor } from '@/lib/table/rows/ordering'
 
 /**
  * A transaction whose anchor lookup finds nothing — the shape a caller produces
@@ -54,3 +54,40 @@ describe('resolveInsertByNeighbor > unknown anchor row', () => {
     expect((error as OrchestrationError).code).toBe('not_found')
   })
 })
+
+/**
+ * An append needs `max(order_key)` and the next `position`, and asks for both in one statement.
+ * Two separate reads were two serial round trips inside the row-order advisory lock, which every
+ * other inserting request on the table is queued behind.
+ */
+describe('appendAnchors', () => {
+  function recordingTrx(row: { maxKey: string | null; maxPos: number }) {
+    const projections: Array<Record<string, unknown>> = []
+    const chain = {
+      select: (projection: Record<string, unknown>) => {
+        projections.push(projection)
+        return chain
+      },
+      from: () => chain,
+      where: async () => [row],
+    }
+    return { trx: chain as unknown as DbTransaction, projections }
+  }
+
+  it('reads both anchors in a single select', async () => {
+    const { trx, projections } = recordingTrx({ maxKey: 'a5', maxPos: 7 })
+
+    const anchors = await appendAnchors(trx, 'table-1')
+
+    expect(projections).toHaveLength(1)
+    expect(Object.keys(projections[0]).sort()).toEqual(['maxKey', 'maxPos'])
+    expect(anchors).toEqual({ maxOrderKey: 'a5', nextPosition: 8 })
+  })
+
+  /** An empty table has no key to append after, and its first row takes position 0. */
+  it('reports the empty table as no key and position zero', async () => {
+    const { trx } = recordingTrx({ maxKey: null, maxPos: -1 })
+
+    expect(await appendAnchors(trx, 'table-1')).toEqual({ maxOrderKey: null, nextPosition: 0 })
+  })
+})

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

@@ -172,6 +172,32 @@ export async function nextRowPosition(trx: DbTransaction, tableId: string): Prom
   return maxPos + 1
 }
 
+/**
+ * The append anchors — `max(order_key)` and the next free `position` — in ONE round trip.
+ *
+ * An append needs both, and asking separately is two serial round trips inside the row-order
+ * advisory lock, which every other inserting request is waiting on. Postgres plans each `max()`
+ * as its own InitPlan, so the combined statement still serves each aggregate from its own index
+ * (`(table_id, order_key, id)` and `(table_id, position)`) with an index-only backward scan —
+ * exactly the two plans the separate queries produced, in one statement rather than two.
+ *
+ * Only for the append case. A positional or neighbor-anchored insert resolves its key by walking
+ * to a slot, which is a different query that cannot fold in.
+ */
+export async function appendAnchors(
+  trx: DbTransaction,
+  tableId: string
+): Promise<{ maxOrderKey: string | null; nextPosition: number }> {
+  const [row] = await trx
+    .select({
+      maxKey: sql<string | null>`max(${userTableRows.orderKey})`,
+      maxPos: sql<number>`coalesce(max(${userTableRows.position}), -1)`.mapWith(Number),
+    })
+    .from(userTableRows)
+    .where(eq(userTableRows.tableId, tableId))
+  return { maxOrderKey: row.maxKey ?? null, nextPosition: row.maxPos + 1 }
+}
+
 /** Largest `order_key` for a table, or `null` when empty — the append anchor for new keys. */
 export async function maxOrderKey(executor: DbOrTx, tableId: string): Promise<string | null> {
   const [{ maxKey }] = await executor
@@ -331,15 +357,27 @@ export async function insertOrderedRow(params: {
     await setTableTxTimeouts(trx)
     await acquireRowOrderLock(trx, tableId)
 
-    // Resolve the authoritative order key from neighbor ids when given, else from
-    // the requested position.
-    const orderKey =
-      afterRowId || beforeRowId
-        ? await resolveInsertByNeighbor(trx, tableId, afterRowId, beforeRowId)
-        : await resolveInsertOrderKey(trx, tableId, position)
-
-    // order_key is authoritative — keep a best-effort, no-shift position.
-    const targetPosition = await nextRowPosition(trx, tableId)
+    // Resolve the authoritative order key from neighbor ids when given, else from the requested
+    // position. `order_key` is authoritative — `position` is a best-effort, no-shift companion.
+    //
+    // A plain append needs only the two table maxima, so it reads them together
+    // ({@link appendAnchors}) rather than paying a second round trip under the order lock. The
+    // anchored and positional forms resolve their key by walking to a slot, so they still ask
+    // for the next position separately.
+    const appending = !afterRowId && !beforeRowId && position === undefined
+    let orderKey: string
+    let targetPosition: number
+    if (appending) {
+      const anchors = await appendAnchors(trx, tableId)
+      orderKey = keyBetween(anchors.maxOrderKey, null)
+      targetPosition = anchors.nextPosition
+    } else {
+      orderKey =
+        afterRowId || beforeRowId
+          ? await resolveInsertByNeighbor(trx, tableId, afterRowId, beforeRowId)
+          : await resolveInsertOrderKey(trx, tableId, position)
+      targetPosition = await nextRowPosition(trx, tableId)
+    }
 
     const rows = await mutateTableRowsWithSecretProvenance(trx, {
       rows: [{ rowId, provenance: secretProvenance }],