Skip to content

Commit 65ddf42

Browse files
committed
refactor(tables): extract selection-to-chip helpers into utils so they are testable
Follow-up I owed on the previous round: the copy path's eligibility rule and the context builder were module-private in a ~4,600-line component with no test file, so the last two fixes to them were reasoned rather than covered — and both were wrong on the first attempt. Moves selectedColumnIds and buildTableSelectionContext to the existing table-grid/utils.ts (which already owns RowSelection, DisplayColumn and getColumnId), and extracts the copy decision as canWriteRowsWithChip. writeLoadedRowsWithChip keeps only the clipboard and toast effects, so the pure rule can be tested without a DOM. utils.ts stays free of side effects. New utils.test.ts covers the two limits that were conflated — a selection past the chip cap still qualifies (the context caps its own rowIds), one past MAX_COPY_ROWS defers to the paged path — plus the all-columns collapse and both caps. Verified to fail against the old chip-cap gate.
1 parent fafe018 commit 65ddf42

3 files changed

Lines changed: 194 additions & 68 deletions

File tree

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx

Lines changed: 16 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,7 @@ import { useParams } from 'next/navigation'
1212
import { usePostHog } from 'posthog-js/react'
1313
import type { RunLimit, RunMode, TableFindMatch } from '@/lib/api/contracts/tables'
1414
import { attachSelectionContextToClipboard } from '@/lib/copilot/chat/selection-clipboard'
15-
import {
16-
buildTableSelectionLabel,
17-
MAX_TABLE_SELECTION_COLUMNS,
18-
MAX_TABLE_SELECTION_ROWS,
19-
} from '@/lib/copilot/chat/selection-context'
15+
import { MAX_TABLE_SELECTION_ROWS } from '@/lib/copilot/chat/selection-context'
2016
import { captureEvent } from '@/lib/posthog/client'
2117
import type {
2218
ColumnDefinition,
@@ -69,7 +65,9 @@ import { AddRowButton, SelectAllCheckbox, TableColGroup } from './table-primitiv
6965
import type { DisplayColumn } from './types'
7066
import {
7167
buildHeaderGroups,
68+
buildTableSelectionContext,
7269
type CellCoord,
70+
canWriteRowsWithChip,
7371
checkboxColLayout,
7472
classifyExecStatusMix,
7573
collectRowSnapshots,
@@ -85,6 +83,7 @@ import {
8583
rowSelectionIncludes,
8684
rowSelectionIsEmpty,
8785
rowSelectionMaterialize,
86+
selectedColumnIds,
8887
} from './utils'
8988

9089
const logger = createLogger('TableView')
@@ -314,63 +313,11 @@ function cellToText(value: unknown, column?: DisplayColumn): string {
314313
return typeof value === 'object' ? JSON.stringify(value) : String(value)
315314
}
316315

317-
/** Column ids spanned by a normalized selection's column range. */
318-
function selectedColumnIds(
319-
columns: DisplayColumn[],
320-
selection: { startCol: number; endCol: number }
321-
): string[] {
322-
const ids: string[] = []
323-
for (let c = selection.startCol; c <= selection.endCol && c < columns.length; c++) {
324-
ids.push(getColumnId(columns[c]))
325-
}
326-
return ids
327-
}
328-
329-
/**
330-
* Materializes a `table_selection` chat context from a grid selection, applying
331-
* the shared row/column caps. `columnIds` narrows the context to a cell range; a
332-
* range covering every column is equivalent to whole rows, so it collapses to an
333-
* open scope (the server then includes all columns, and stays correct if the
334-
* schema changes). Returns null before the table name has loaded or when nothing
335-
* is selected.
336-
*/
337-
function buildTableSelectionContext(opts: {
338-
tableId: string
339-
tableName: string | undefined
340-
totalColumnCount: number
341-
rowIds: string[]
342-
columnIds?: string[]
343-
}): ChatContext | null {
344-
const { tableId, tableName, totalColumnCount, columnIds } = opts
345-
if (!tableName || opts.rowIds.length === 0) return null
346-
const rowIds = opts.rowIds.slice(0, MAX_TABLE_SELECTION_ROWS)
347-
const scopedColumnIds =
348-
columnIds && columnIds.length > 0 && columnIds.length < totalColumnCount
349-
? columnIds.slice(0, MAX_TABLE_SELECTION_COLUMNS)
350-
: undefined
351-
return {
352-
kind: 'table_selection',
353-
tableId,
354-
tableName,
355-
label: buildTableSelectionLabel(tableName, rowIds.length, scopedColumnIds?.length),
356-
rowIds,
357-
...(scopedColumnIds ? { columnIds: scopedColumnIds } : {}),
358-
}
359-
}
360-
361316
/**
362317
* Copies `rows` synchronously on the copy event so a chat-selection chip can
363-
* ride alongside the tab-separated text. The paged `writeSelectionToClipboard`
364-
* cannot do this: its async Clipboard API write replaces the whole clipboard and
365-
* so cannot carry a custom MIME type.
366-
*
367-
* Taking this path requires a chip to carry, `rows` being the complete copy, and
368-
* a set within the chip cap; otherwise the canonical paged path handles the
369-
* copy, including its row loading and truncation notice.
318+
* ride alongside the tab-separated text. Eligibility lives in
319+
* {@link canWriteRowsWithChip}; this owns only the clipboard and toast effects.
370320
*
371-
* @param complete - Whether `rows` is everything this copy should contain. False
372-
* when the paged path would load rows this caller cannot see yet, so deferring
373-
* to it copies strictly more.
374321
* @returns True when it handled the copy, false to fall through to the paged path.
375322
*/
376323
function writeLoadedRowsWithChip(opts: {
@@ -381,15 +328,16 @@ function writeLoadedRowsWithChip(opts: {
381328
context: ChatContext | null
382329
}): boolean {
383330
const { rows, context } = opts
384-
// Bounded by the TEXT limit, not the chip's row cap: `context` already slices
385-
// itself to MAX_TABLE_SELECTION_ROWS, so a larger selection still copies in
386-
// full here and carries a chip for as many rows as a chip can reference —
387-
// matching what Add to Chat does with the same selection. Gating on the chip
388-
// cap instead would drop the chip entirely on the async fall-through, which
389-
// cannot carry a custom MIME. Past MAX_COPY_ROWS the paged path must take
390-
// over, since it owns truncation and the notice that goes with it.
391-
if (!context || !opts.complete || rows.length === 0) return false
392-
if (rows.length > TABLE_LIMITS.MAX_COPY_ROWS) return false
331+
if (
332+
!canWriteRowsWithChip({
333+
rowCount: rows.length,
334+
complete: opts.complete,
335+
hasContext: Boolean(context),
336+
}) ||
337+
!context
338+
) {
339+
return false
340+
}
393341
opts.clipboardData?.setData(
394342
'text/plain',
395343
rows.map((row) => opts.buildCells(row).join('\t')).join('\n')
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
MAX_TABLE_SELECTION_COLUMNS,
7+
MAX_TABLE_SELECTION_ROWS,
8+
} from '@/lib/copilot/chat/selection-context'
9+
import { TABLE_LIMITS } from '@/lib/table/constants'
10+
import type { DisplayColumn } from './types'
11+
import { buildTableSelectionContext, canWriteRowsWithChip, selectedColumnIds } from './utils'
12+
13+
function columns(count: number): DisplayColumn[] {
14+
return Array.from({ length: count }, (_, i) => ({
15+
id: `c${i}`,
16+
name: `Col ${i}`,
17+
})) as unknown as DisplayColumn[]
18+
}
19+
20+
const rowIds = (count: number) => Array.from({ length: count }, (_, i) => `r${i}`)
21+
22+
describe('selectedColumnIds', () => {
23+
it('returns the ids the range spans', () => {
24+
expect(selectedColumnIds(columns(5), { startCol: 1, endCol: 3 })).toEqual(['c1', 'c2', 'c3'])
25+
})
26+
27+
it('stops at the last column when the range overruns', () => {
28+
expect(selectedColumnIds(columns(2), { startCol: 0, endCol: 9 })).toEqual(['c0', 'c1'])
29+
})
30+
})
31+
32+
describe('buildTableSelectionContext', () => {
33+
const base = { tableId: 't1', tableName: 'Sales', totalColumnCount: 3 }
34+
35+
it('returns null before the table name has loaded, or with nothing selected', () => {
36+
expect(buildTableSelectionContext({ ...base, tableName: undefined, rowIds: ['r1'] })).toBeNull()
37+
expect(buildTableSelectionContext({ ...base, rowIds: [] })).toBeNull()
38+
})
39+
40+
it('caps rows at the chip limit and labels the capped count, not the requested one', () => {
41+
const context = buildTableSelectionContext({
42+
...base,
43+
rowIds: rowIds(MAX_TABLE_SELECTION_ROWS + 250),
44+
})
45+
46+
expect(context?.kind).toBe('table_selection')
47+
if (context?.kind !== 'table_selection') throw new Error('expected a table_selection')
48+
expect(context.rowIds).toHaveLength(MAX_TABLE_SELECTION_ROWS)
49+
expect(context.label).toContain(`${MAX_TABLE_SELECTION_ROWS} rows`)
50+
})
51+
52+
it('collapses a range covering every column to an open scope', () => {
53+
// Equivalent to whole rows — leaving it open keeps the server correct if the
54+
// schema changes, instead of pinning a now-stale column list.
55+
const context = buildTableSelectionContext({
56+
...base,
57+
rowIds: ['r1'],
58+
columnIds: ['c0', 'c1', 'c2'],
59+
})
60+
61+
if (context?.kind !== 'table_selection') throw new Error('expected a table_selection')
62+
expect(context.columnIds).toBeUndefined()
63+
})
64+
65+
it('keeps a narrower range scoped, capped at the column limit', () => {
66+
const context = buildTableSelectionContext({
67+
...base,
68+
totalColumnCount: MAX_TABLE_SELECTION_COLUMNS + 50,
69+
rowIds: ['r1'],
70+
columnIds: Array.from({ length: MAX_TABLE_SELECTION_COLUMNS + 10 }, (_, i) => `c${i}`),
71+
})
72+
73+
if (context?.kind !== 'table_selection') throw new Error('expected a table_selection')
74+
expect(context.columnIds).toHaveLength(MAX_TABLE_SELECTION_COLUMNS)
75+
})
76+
})
77+
78+
describe('canWriteRowsWithChip', () => {
79+
const ok = { rowCount: 10, complete: true, hasContext: true }
80+
81+
it('allows a complete, in-bounds selection that has a chip to carry', () => {
82+
expect(canWriteRowsWithChip(ok)).toBe(true)
83+
})
84+
85+
it('defers when there is no chip, nothing selected, or the paged path would load more', () => {
86+
expect(canWriteRowsWithChip({ ...ok, hasContext: false })).toBe(false)
87+
expect(canWriteRowsWithChip({ ...ok, rowCount: 0 })).toBe(false)
88+
expect(canWriteRowsWithChip({ ...ok, complete: false })).toBe(false)
89+
})
90+
91+
it('stays allowed past the chip row cap — the context caps itself', () => {
92+
// Gating on MAX_TABLE_SELECTION_ROWS here would drop the chip entirely on
93+
// the async fall-through, while Add to Chat on the same selection still
94+
// produces a capped chip.
95+
expect(canWriteRowsWithChip({ ...ok, rowCount: MAX_TABLE_SELECTION_ROWS + 100 })).toBe(true)
96+
})
97+
98+
it('defers past the text copy limit, which owns truncation', () => {
99+
expect(canWriteRowsWithChip({ ...ok, rowCount: TABLE_LIMITS.MAX_COPY_ROWS })).toBe(true)
100+
expect(canWriteRowsWithChip({ ...ok, rowCount: TABLE_LIMITS.MAX_COPY_ROWS + 1 })).toBe(false)
101+
})
102+
})

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/utils.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
11
import type { ActiveDispatch } from '@/lib/api/contracts/tables'
2+
import {
3+
buildTableSelectionLabel,
4+
MAX_TABLE_SELECTION_COLUMNS,
5+
MAX_TABLE_SELECTION_ROWS,
6+
} from '@/lib/copilot/chat/selection-context'
27
import type {
38
ColumnDefinition,
49
RowExecutionMetadata,
@@ -7,7 +12,9 @@ import type {
712
WorkflowGroup,
813
} from '@/lib/table'
914
import { getColumnId } from '@/lib/table/column-keys'
15+
import { TABLE_LIMITS } from '@/lib/table/constants'
1016
import { areGroupDepsSatisfied, areOutputsFilled } from '@/lib/table/deps'
17+
import type { ChatContext } from '@/stores/panel'
1118
import type { DeletedRowSnapshot } from '@/stores/table/types'
1219
import type { DisplayColumn } from './types'
1320

@@ -351,3 +358,72 @@ export function collectRowSnapshots(rows: Iterable<TableRowType>): DeletedRowSna
351358
}
352359
return snapshots
353360
}
361+
362+
/** Column ids spanned by a normalized selection's column range. */
363+
export function selectedColumnIds(
364+
columns: DisplayColumn[],
365+
selection: { startCol: number; endCol: number }
366+
): string[] {
367+
const ids: string[] = []
368+
for (let c = selection.startCol; c <= selection.endCol && c < columns.length; c++) {
369+
ids.push(getColumnId(columns[c]))
370+
}
371+
return ids
372+
}
373+
374+
/**
375+
* Materializes a `table_selection` chat context from a grid selection, applying
376+
* the shared row/column caps. `columnIds` narrows the context to a cell range; a
377+
* range covering every column is equivalent to whole rows, so it collapses to an
378+
* open scope (the server then includes all columns, and stays correct if the
379+
* schema changes). Returns null before the table name has loaded or when nothing
380+
* is selected.
381+
*/
382+
export function buildTableSelectionContext(opts: {
383+
tableId: string
384+
tableName: string | undefined
385+
totalColumnCount: number
386+
rowIds: string[]
387+
columnIds?: string[]
388+
}): ChatContext | null {
389+
const { tableId, tableName, totalColumnCount, columnIds } = opts
390+
if (!tableName || opts.rowIds.length === 0) return null
391+
const rowIds = opts.rowIds.slice(0, MAX_TABLE_SELECTION_ROWS)
392+
const scopedColumnIds =
393+
columnIds && columnIds.length > 0 && columnIds.length < totalColumnCount
394+
? columnIds.slice(0, MAX_TABLE_SELECTION_COLUMNS)
395+
: undefined
396+
return {
397+
kind: 'table_selection',
398+
tableId,
399+
tableName,
400+
label: buildTableSelectionLabel(tableName, rowIds.length, scopedColumnIds?.length),
401+
rowIds,
402+
...(scopedColumnIds ? { columnIds: scopedColumnIds } : {}),
403+
}
404+
}
405+
406+
/**
407+
* Whether a copy can be written synchronously on the event — the only way a
408+
* chat-selection chip survives, since the paged path's async Clipboard API write
409+
* replaces the whole clipboard and cannot carry a custom MIME type.
410+
*
411+
* Bounded by the TEXT limit, not the chip's row cap: a context slices its own
412+
* `rowIds` to {@link MAX_TABLE_SELECTION_ROWS}, so a larger selection should
413+
* still copy in full here and carry a chip for as many rows as a chip can
414+
* reference — matching what Add to Chat does with the same selection. Gating on
415+
* the chip cap instead drops the chip entirely. Past `MAX_COPY_ROWS` the paged
416+
* path must take over, because it owns truncation and its user-facing notice.
417+
*
418+
* @param complete - Whether the caller's rows are everything the copy should
419+
* contain. False when the paged path would load rows the caller cannot see yet,
420+
* so deferring to it copies strictly more.
421+
*/
422+
export function canWriteRowsWithChip(opts: {
423+
rowCount: number
424+
complete: boolean
425+
hasContext: boolean
426+
}): boolean {
427+
if (!opts.hasContext || !opts.complete) return false
428+
return opts.rowCount > 0 && opts.rowCount <= TABLE_LIMITS.MAX_COPY_ROWS
429+
}

0 commit comments

Comments
 (0)