refactor(table): apply review findings from the quality pass

Four parallel reviews (reuse, simplification, efficiency, altitude) converged
on the same set. Applied:

Removed actorClientId entirely. It had no supplier anywhere in the repo, so
every call reached signalTableRowsChangedByActor(id, undefined), which is
byte-identical to the broadcast it replaced — three optional fields, three
verbatim doc blocks and a pin test asserting the empty set, all inert. It
belongs with the route migration that supplies an actor. The attribution pin
is restored to its original form.

The uniqueness probe was only half-narrowed: the patched column list was
computed and then discarded, and the probe re-derived every unique column from
the full schema. It now receives only the columns the patch touched, so a
table with several unique columns runs one query instead of all of them.

assertKnownColumnIds hand-rolled an id index and duplicated the sibling
assert's message verbatim. It now reuses buildColumnNameById — which already
keys by getColumnId, so the legacy pre-backfill column case is handled by the
shared helper rather than by a special case here — and both asserts share one
message builder.

The job field list had become two copies, one drizzle-checked and one an
unchecked sql<T> cast that could silently return undefined for a renamed
field. The lateral now derives its jsonb pairs from JOB_PROJECTION, which
satisfies Record<keyof LatestJobRow, Column>. That compile-time guarantee
replaces the runtime drift test it makes redundant.

Also: hoisted the id index out of the batch loop to match the names path,
dropped a never-supplied parameter, narrowed an over-broad parameter type,
removed a dead timer cleanup and its now-unused import, replaced dynamic
re-imports with the static one already present, hoisted a repeated stub, and
documented why filters need no keying counterpart and how laxness differs
between the two wires.

Verified to fail: removing a field from JOB_PROJECTION breaks the build in two
places.

Author: waleedlatif1

apps/sim/lib/table/__tests__/update-row.test.ts

@@ -14,7 +14,7 @@ import {
   upsertRow,
 } from '@/lib/table/rows/service'
 import type { TableDefinition } from '@/lib/table/types'
-import { getUniqueColumns } from '@/lib/table/validation'
+import { checkUniqueConstraintsDb, getUniqueColumns } from '@/lib/table/validation'
 
 // Capacity is exercised in billing.test.ts; here it's a no-op so the timeout-scaling
 // suites can use large synthetic row counts without tripping the plan limit.
@@ -563,16 +563,16 @@ describe('updateRow — uniqueness probe scoping', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     resetDbChainMock()
+    // The common case: one unique column. The two tests that need a different
+    // shape override this.
+    vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
     dbChainMockFns.limit.mockResolvedValue([EXISTING_ROW])
     dbChainMockFns.returning.mockResolvedValue([
       { id: EXISTING_ROW.id, updatedAt: PERSISTED_UPDATED_AT },
     ])
   })
 
   it('does not probe when the patch touches no unique column', async () => {
-    const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
-    vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
-
     await updateRow(
       { tableId: 'tbl-1', rowId: 'row-1', data: { age: 31 }, workspaceId: 'ws-1' },
       TABLE,
@@ -583,9 +583,6 @@ describe('updateRow — uniqueness probe scoping', () => {
   })
 
   it('still probes when the patch touches a unique column', async () => {
-    const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
-    vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
-
     await updateRow(
       { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
       TABLE,
@@ -596,25 +593,40 @@ describe('updateRow — uniqueness probe scoping', () => {
   })
 
   it('probes against the merged row, so the excluded row is still the one being edited', async () => {
-    const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
-    vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
-
     await updateRow(
       { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
       TABLE,
       'req-1'
     )
 
+    // The merged row is what gets probed, the excluded row is the one being
+    // edited, and the schema is narrowed to the unique columns this patch
+    // touched — the probe issues one query per column it is handed.
     expect(checkUniqueConstraintsDb).toHaveBeenCalledWith(
       'tbl-1',
       { name: 'Grace', age: 30 },
-      TABLE.schema,
+      { ...TABLE.schema, columns: [{ name: 'name', type: 'string', unique: true }] },
       'row-1'
     )
   })
 
+  it('hands the probe only the unique columns the patch touched', async () => {
+    vi.mocked(getUniqueColumns).mockReturnValue([
+      { name: 'name', type: 'string', unique: true },
+      { name: 'email', type: 'string', unique: true },
+    ])
+
+    await updateRow(
+      { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
+      TABLE,
+      'req-1'
+    )
+
+    const schemaArg = vi.mocked(checkUniqueConstraintsDb).mock.calls[0][2]
+    expect(schemaArg.columns).toEqual([{ name: 'name', type: 'string', unique: true }])
+  })
+
   it('surfaces a duplicate on a column the patch does write', async () => {
-    const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
     vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
     vi.mocked(checkUniqueConstraintsDb).mockResolvedValueOnce({
       valid: false,
@@ -631,7 +643,6 @@ describe('updateRow — uniqueness probe scoping', () => {
   })
 
   it('does not probe on a table with no unique columns at all', async () => {
-    const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
     vi.mocked(getUniqueColumns).mockReturnValue([])
 
     await updateRow(

apps/sim/lib/table/application/context.test.ts

@@ -2,7 +2,7 @@
  * @vitest-environment node
  */
 
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const { getTableById, loadWorkspace } = vi.hoisted(() => ({
   getTableById: vi.fn(),
@@ -60,10 +60,6 @@ describe('table application context', () => {
     )
   })
 
-  afterEach(() => {
-    vi.useRealTimers()
-  })
-
   it('derives workspace scope from the canonical active table', async () => {
     await expect(
       resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' })

apps/sim/lib/table/application/rows.test.ts

@@ -19,7 +19,6 @@ const {
   mockResolveContext,
   mockResolvePermission,
   mockSignalRowsChanged,
-  mockSignalRowsChangedByActor,
   mockUpsertRow,
   mockWithLockedTable,
   mockInsertRow,
@@ -42,7 +41,6 @@ const {
   mockResolveContext: vi.fn(),
   mockResolvePermission: vi.fn(),
   mockSignalRowsChanged: vi.fn(),
-  mockSignalRowsChangedByActor: vi.fn(),
   mockUpsertRow: vi.fn(),
   mockWithLockedTable: vi.fn(),
   mockInsertRow: vi.fn(),
@@ -139,12 +137,10 @@ vi.mock('@/lib/table/application/context', () => ({
 
 vi.mock('@/lib/table/events', () => ({
   signalTableRowsChanged: mockSignalRowsChanged,
-  signalTableRowsChangedByActor: mockSignalRowsChangedByActor,
 }))
 
 import {
   createTableRows,
-  deleteTableRow,
   deleteTableRows,
   listTableRows,
   ProjectedWireRowsValidationError,
@@ -1297,85 +1293,3 @@ describe('row data keying', () => {
     ).rejects.toThrow(/Row 2: Unknown columns: zzz, qqq/)
   })
 })
-
-/**
- * Naming the acting tab lets that tab skip refetching its own write. Only the
- * single-row paths accept an actor — see `events.attribution.test.ts` for why,
- * and for the pinned list of surfaces allowed to supply one.
- */
-describe('row change attribution', () => {
-  beforeEach(() => {
-    vi.clearAllMocks()
-    mockResolvePermission.mockResolvedValue('write')
-    mockResolveContext.mockResolvedValue({
-      tableId: TABLE.id,
-      table: TABLE,
-      workspaceId: TABLE.workspaceId,
-      workspaceOrganizationId: 'organization-1',
-      allowPersonalApiKeys: true,
-      billedAccountUserId: 'billing-owner-1',
-    })
-    mockAssertRowCapacity.mockResolvedValue(10_000)
-    mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} })
-    mockIsScopeCompatible.mockReturnValue(true)
-  })
-
-  it('names the acting tab on a single-row update', async () => {
-    await updateTableRow.execute({
-      principal: PRINCIPAL,
-      input: {
-        tableId: TABLE.id,
-        rowId: 'row-1',
-        data: { name: 'Ada' },
-        strictWrite: false,
-        dataKeying: 'names',
-        actorClientId: 'tab-42',
-      },
-    })
-
-    expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42')
-    expect(mockSignalRowsChanged).not.toHaveBeenCalled()
-  })
-
-  it('broadcasts to everyone when the surface cannot name a tab', async () => {
-    await updateTableRow.execute({
-      principal: PRINCIPAL,
-      input: {
-        tableId: TABLE.id,
-        rowId: 'row-1',
-        data: { name: 'Ada' },
-        strictWrite: false,
-        dataKeying: 'names',
-      },
-    })
-
-    expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, undefined)
-  })
-
-  it('names the acting tab on a single-row delete', async () => {
-    await deleteTableRow.execute({
-      principal: PRINCIPAL,
-      input: { tableId: TABLE.id, rowId: 'row-1', actorClientId: 'tab-42' },
-    })
-
-    expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42')
-  })
-
-  it('broadcasts a batch insert to everyone even though a tab is named', async () => {
-    await createTableRows.execute({
-      principal: PRINCIPAL,
-      input: {
-        kind: 'batch',
-        tableId: TABLE.id,
-        rows: [{ name: 'Ada' }, { name: 'Grace' }],
-        strictWrite: false,
-        dataKeying: 'names',
-      },
-    })
-
-    // A batch write is not reconciled locally by the acting tab, so it must
-    // refetch like everyone else.
-    expect(mockSignalRowsChanged).toHaveBeenCalledWith(TABLE.id)
-    expect(mockSignalRowsChangedByActor).not.toHaveBeenCalled()
-  })
-})