Skip to content

Commit 4cf93cb

Browse files
committed
fix(tables): make the retype gate and the write path share one parser
A simplify pass over the registry found two real defects and several places the abstraction was being worked around. Silent data loss on conversion. `isCompatibleWith` was hand-written per type and had already drifted from `coerce`, despite the interface promising they could not: `boolean` accepted '1'/'0'/0/1 in the gate but only 'true'/'false' in the write path, so converting a column holding "1" reported zero incompatible rows and then nulled every one of them. `date` drifted the other way. `isCompatibleWith` is now optional and defaults to `coerce(...).ok`, so the two are the same code; only `select` overrides, because its rules are about the column (cleared-vs-required, cardinality) not the value. `isColumnType` used `in`, which matches inherited keys — `isColumnType('toString')` was true and `columnTypeById('toString')` returned `Function.prototype.toString`, which the validator would then call `.validateDefinition()` on. Now `Object.hasOwn`. `defaultMetadata` only ran on the currency arm of a retype, so a future type would get its defaults on create but silently not on conversion. It now runs for every non-select target, carrying forward only metadata the TARGET type declares it owns — a currency→text conversion no longer strands a currencyCode. The index doc claimed the registry is kept out of the `@/lib/table` barrel so 44 server modules don't pull `@sim/emcn/icons`. That was false: `constants.ts` re-exported `COLUMN_TYPES` from the icon-carrying `registry.ts`, and the barrel re-exports `constants`. `COLUMN_TYPES` now lives in the icon-free `types.ts`; verified with an import tracer that both are icon-free again. Also: 5 no-op `validateDefinition`s and 4 duplicated formatters collapsed into registry defaults; `CURRENCY_OPTIONS` was an eager module-load IIFE costing ~8ms of ICU work on every table API route for a list only the config sidebar reads, now built on first call; and the skill's validation grep claimed 'should return nothing' when it returns 8 legitimate hits — it now explains how to tell a leak from a genuine special case.
1 parent aba4433 commit 4cf93cb

18 files changed

Lines changed: 111 additions & 113 deletions

File tree

.agents/skills/add-column-type/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
129129
## Final Validation (Required)
130130

131131
1. **`cd apps/sim && bunx tsc --noEmit -p tsconfig.json`** — must be clean. If any file *outside* `column-types/` errors, that file has a hardcoded type list; fix it to read the registry.
132-
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/` should return nothing but genuine JSX/behavioral dispatch.
132+
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/`. Hits are expected; judge each. A hit is fine when it mounts a specific React component or encodes a genuinely one-off behavior (`json`'s mono textarea, `date`'s timezone-aware parsing). A hit is a **leak** when it restates something the registry could answer — an icon, a label, a colour, an operator set, a cast, a coercion. Leaks get a registry field, not a new branch.
133133
3. **Run the suite**`bunx vitest run lib/table 'app/workspace/[workspaceId]/tables' lib/api app/api/table app/api/v1 lib/copilot/tools/server/table`. Existing tests must pass **unchanged**; needing to edit one means you changed behavior for the other types.
134134
4. **`bun run lint:check`, `bun run check:api-validation`, `bun run check:client-boundary`** from the repo root.
135135
5. **Exercise it in the running app** on a table with one column of every type: create, edit inline / in the expanded popover / in the row modal, paste from a spreadsheet, filter, sort, convert to and from other types, export CSV, undo a column delete.

.claude/commands/add-column-type.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
128128
## Final Validation (Required)
129129

130130
1. **`cd apps/sim && bunx tsc --noEmit -p tsconfig.json`** — must be clean. If any file *outside* `column-types/` errors, that file has a hardcoded type list; fix it to read the registry.
131-
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/` should return nothing but genuine JSX/behavioral dispatch.
131+
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/`. Hits are expected; judge each. A hit is fine when it mounts a specific React component or encodes a genuinely one-off behavior (`json`'s mono textarea, `date`'s timezone-aware parsing). A hit is a **leak** when it restates something the registry could answer — an icon, a label, a colour, an operator set, a cast, a coercion. Leaks get a registry field, not a new branch.
132132
3. **Run the suite**`bunx vitest run lib/table 'app/workspace/[workspaceId]/tables' lib/api app/api/table app/api/v1 lib/copilot/tools/server/table`. Existing tests must pass **unchanged**; needing to edit one means you changed behavior for the other types.
133133
4. **`bun run lint:check`, `bun run check:api-validation`, `bun run check:client-boundary`** from the repo root.
134134
5. **Exercise it in the running app** on a table with one column of every type: create, edit inline / in the expanded popover / in the row modal, paste from a spreadsheet, filter, sort, convert to and from other types, export CSV, undo a column delete.

.cursor/commands/add-column-type.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
123123
## Final Validation (Required)
124124

125125
1. **`cd apps/sim && bunx tsc --noEmit -p tsconfig.json`** — must be clean. If any file *outside* `column-types/` errors, that file has a hardcoded type list; fix it to read the registry.
126-
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/` should return nothing but genuine JSX/behavioral dispatch.
126+
2. **Grep for leaks**`grep -rn "=== '{id}'" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/`. Hits are expected; judge each. A hit is fine when it mounts a specific React component or encodes a genuinely one-off behavior (`json`'s mono textarea, `date`'s timezone-aware parsing). A hit is a **leak** when it restates something the registry could answer — an icon, a label, a colour, an operator set, a cast, a coercion. Leaks get a registry field, not a new branch.
127127
3. **Run the suite**`bunx vitest run lib/table 'app/workspace/[workspaceId]/tables' lib/api app/api/table app/api/v1 lib/copilot/tools/server/table`. Existing tests must pass **unchanged**; needing to edit one means you changed behavior for the other types.
128128
4. **`bun run lint:check`, `bun run check:api-validation`, `bun run check:client-boundary`** from the repo root.
129129
5. **Exercise it in the running app** on a table with one column of every type: create, edit inline / in the expanded popover / in the row modal, paste from a spreadsheet, filter, sort, convert to and from other types, export CSV, undo a column delete.

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ import { X } from '@sim/emcn/icons'
66
import { toError } from '@sim/utils/errors'
77
import { findValidationIssue, isValidationError } from '@/lib/api/client/errors'
88
import type { ColumnDefinition, SelectOption } from '@/lib/table'
9-
import { CURRENCY_OPTIONS, DEFAULT_CURRENCY_CODE, resolveCurrencyCode } from '@/lib/table/currency'
9+
import {
10+
DEFAULT_CURRENCY_CODE,
11+
getCurrencyOptions,
12+
resolveCurrencyCode,
13+
} from '@/lib/table/currency'
1014
import {
1115
FieldError,
1216
RequiredLabel,
@@ -24,7 +28,7 @@ function isSelectType(type: ColumnDefinition['type']): boolean {
2428
* Picker entries, built once at module load: the option list is derived from the
2529
* runtime's currency data and never varies per column.
2630
*/
27-
const CURRENCY_COMBOBOX_OPTIONS = CURRENCY_OPTIONS.map((c) => ({
31+
const CURRENCY_COMBOBOX_OPTIONS = getCurrencyOptions().map((c) => ({
2832
value: c.code,
2933
label: `${c.code} · ${c.name}`,
3034
}))

apps/sim/lib/table/__tests__/currency.test.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
*/
88
import { describe, expect, it } from 'vitest'
99
import {
10-
CURRENCY_OPTIONS,
1110
DEFAULT_CURRENCY_CODE,
1211
formatCurrencyDisplay,
1312
formatCurrencyForInput,
13+
getCurrencyOptions,
1414
isSupportedCurrencyCode,
1515
parseCurrencyInput,
1616
resolveCurrencyCode,
@@ -163,8 +163,9 @@ describe('currency codes', () => {
163163
})
164164

165165
it('offers the pinned codes first and no duplicates', () => {
166-
expect(CURRENCY_OPTIONS[0].code).toBe('USD')
167-
const codes = CURRENCY_OPTIONS.map((c) => c.code)
166+
const options = getCurrencyOptions()
167+
expect(options[0].code).toBe('USD')
168+
const codes = options.map((c) => c.code)
168169
expect(new Set(codes).size).toBe(codes.length)
169170
expect(codes).toContain('JPY')
170171
})

apps/sim/lib/table/column-types/boolean.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,6 @@ export const booleanColumnType: ColumnTypeDefinition = {
3131
return typeof value === 'boolean' ? null : `${column.name} must be boolean`
3232
},
3333

34-
validateDefinition() {
35-
return []
36-
},
37-
38-
isCompatibleWith(value) {
39-
if (typeof value === 'boolean') return true
40-
if (typeof value === 'string') return ['true', 'false', '1', '0'].includes(value.toLowerCase())
41-
if (typeof value === 'number') return value === 0 || value === 1
42-
return false
43-
},
44-
4534
formatForDisplay(value) {
4635
return String(value)
4736
},

apps/sim/lib/table/column-types/currency.ts

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,6 @@ export const currencyColumnType: ColumnTypeDefinition = {
5252
return []
5353
},
5454

55-
isCompatibleWith(value) {
56-
// Read the value exactly as the write-path coercion will: a formatted
57-
// amount left in a text column (`$1,234.56`) converts, arbitrary prose
58-
// does not.
59-
return parseCurrencyInput(value) !== null
60-
},
61-
6255
formatForDisplay(value, column) {
6356
return formatCurrencyDisplay(value, column.currencyCode)
6457
},

apps/sim/lib/table/column-types/date.ts

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,6 @@ export const dateColumnType: ColumnTypeDefinition = {
4242
return valid ? null : `${column.name} must be valid date`
4343
},
4444

45-
validateDefinition() {
46-
return []
47-
},
48-
49-
isCompatibleWith(value) {
50-
if (value instanceof Date) return !Number.isNaN(value.getTime())
51-
if (typeof value === 'string') return !Number.isNaN(Date.parse(value))
52-
return false
53-
},
54-
5545
formatForDisplay(value) {
5646
return formatDateCellDisplay(String(value), { seconds: true })
5747
},

apps/sim/lib/table/column-types/json.ts

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,6 @@ export const jsonColumnType: ColumnTypeDefinition = {
2929
}
3030
},
3131

32-
validateDefinition() {
33-
return []
34-
},
35-
36-
isCompatibleWith() {
37-
return true
38-
},
39-
4032
formatForDisplay(value) {
4133
return JSON.stringify(value)
4234
},

apps/sim/lib/table/column-types/number.ts

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,6 @@ export const numberColumnType: ColumnTypeDefinition = {
3535
: `${column.name} must be number`
3636
},
3737

38-
validateDefinition() {
39-
return []
40-
},
41-
42-
isCompatibleWith(value) {
43-
if (typeof value === 'number') return Number.isFinite(value)
44-
if (typeof value === 'string') {
45-
const num = Number(value)
46-
return Number.isFinite(num) && value.trim() !== ''
47-
}
48-
return false
49-
},
50-
5138
formatForDisplay(value) {
5239
return String(value)
5340
},

0 commit comments

Comments
 (0)