Skip to content

Commit 13a2326

Browse files
committed
fix(tables): refuse conversions that would invent or destroy values
Final adversarial scan found two data-corrupting conversions, both opened by defaulting the retype gate to the write path's parser. number → date destroyed every value. `date.coerce` reads a number as epoch milliseconds, which is right for one deliberate write and catastrophic applied to a whole column: 1, 5, 42 became three timestamps in January 1970, and a Unix-seconds column landed in 1970 rather than the year it meant. Irreversible. `date` now overrides the gate to reject numbers, restoring the pre-refactor behavior, and the contract states the rule the override obeys: a gate may be STRICTER than `coerce`, never looser. Stricter refuses a bulk conversion while single writes still work; looser is the direction that corrupts. string → currency invented values. The parser stripped every non-digit and joined what was left, so `01/02/2024` read as 1022024, `Room 101` as 101, and `0.1.2` as 12 — a column of SKUs or phone numbers converted with zero reported incompatibilities. What remains after removing symbols, spacing and an ISO code must now be only digits and separators, and grouping must be well-formed (a first group of 1-3 digits, the rest exactly 3). Every legitimate form still parses, including all the locale variants. Also generifies the last three metadata leaks: `buildConvertedColumn` strips and carries back by iterating the key list rather than naming keys (naming them meant a future type's metadata rode onto a target that rejects it, failing that column's validation on every later write), `normalizeColumn` forwards metadata through a shared `typeMetadataOf`, and `filterOperatorsFor` moved onto the definition — it was a per-type branch inside the registry's own accessor, the one thing the registry exists to forbid. Skill corrected: it claimed COLUMN_TYPES derives from the registry (backwards), promised exactly two compile errors (four once a type owns metadata), used a grep that missed half the real branches, and never mentioned `import.ts`'s second coercion path, whose silent default arm is the costliest miss available. Differential re-run vs the pre-refactor implementations: 0 coercion differences, 1 intentional compatibility difference (boolean no longer accepts the 0/1 conversions the old gate accepted and then nulled).
1 parent f69197e commit 13a2326

17 files changed

Lines changed: 246 additions & 74 deletions

File tree

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

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@ Do **not** hunt for places to edit. Add your type to the `ColumnType` union firs
1818
cd apps/sim && bunx tsc --noEmit -p tsconfig.json
1919
```
2020

21-
You will get exactly two errors, naming `column-types/registry.ts` and `column-types/registry.server.ts`. Those are the only two files you must register in. If you get a third error somewhere else, that site is reading a hardcoded type list that should be reading the registry — fix that site, don't work around it.
21+
You will get two errors, naming `column-types/registry.ts` and `column-types/registry.server.ts`. Register in both.
22+
23+
If your type owns metadata, adding its key to `TYPE_SPECIFIC_COLUMN_KEYS` produces two more legitimate errors — `FOREIGN_METADATA_VERB` in `validation.ts` (a `Record` over those keys) and the key's absence from `ColumnDefinition`. Those are the gate working, not sites to "fix".
24+
25+
Any error beyond those four is a site reading a hardcoded type list that should read the registry — fix that site, don't work around it.
2226

2327
## Directory Structure
2428

@@ -92,7 +96,9 @@ The three that are easy to get wrong:
9296

9397
## Step 4: Register
9498

95-
Add the entry to `COLUMN_TYPE_REGISTRY` in `registry.ts` **and** `COLUMN_TYPE_SERVER_REGISTRY` in `registry.server.ts`. `COLUMN_TYPES` derives from the registry keys, so `constants.ts`, the zod contract, and every validator pick it up with no edit.
99+
Add the entry to `COLUMN_TYPE_REGISTRY` in `registry.ts` **and** `COLUMN_TYPE_SERVER_REGISTRY` in `registry.server.ts`.
100+
101+
`COLUMN_TYPES` is declared in `types.ts` (not derived from the registry — the registry is annotated `Record<ColumnType, …>` against it, which is the gate). `constants.ts` re-exports it, so `columnTypeSchema = z.enum(COLUMN_TYPES)` picks your type up with no edit. **Type-specific metadata does not** — see the next step.
96102

97103
## Step 5: Migrations (only if the stored bytes change)
98104

@@ -113,8 +119,27 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
113119
- **Import cycles.** `column-types/select.ts` imports `select-values.ts`, so `select-values.ts` must **not** import the registry — that closes a cycle and fails at module init. Inside a type's own helper module the string literal is the implementation, not a config leak.
114120
- **The client-safe boundary.** `registry.ts` and everything it imports must stay free of `@sim/db`, `drizzle-orm`, and `next/server` — the tables grid imports it directly. A React icon is fine (it's a component *reference*, never called server-side). Only `registry.server.ts` may touch drizzle.
115121
- **Don't re-export the registry from `@/lib/table`.** 44 server modules import that barrel; routing this through it pulls `@sim/emcn/icons` into all of them. Deep-import `@/lib/table/column-types`.
122+
- **`import.ts`'s `coerceValue` is a SECOND write path and is not opt-in.** Importing into a column of your type always hits it, and its `default` arm silently `String(value)`s — so a missing `case` stores text in a column whose `jsonbCast` is numeric, and then every filter and sort on that column errors in Postgres. Add a `case`, even though the switch compiles without one. (It is deliberately separate from the registry's `coerce`: an import wants an unparseable value to survive as its raw string so the row error can name it.)
116123
- **CSV inference** is an ordered heuristic in `import.ts`, deliberately not registry-driven. A new type is not inferred from a CSV unless you extend `inferColumnType` — usually you should not, since inference cannot supply configuration (an option set, a currency code).
117124

125+
## If your type owns metadata, read this
126+
127+
Registering the *type* is compiler-enforced. Registering its *metadata* is not, and that is where the remaining manual work lives. A key like `precision` has to be added in each of these, none of which will fail to compile if you forget:
128+
129+
| Where | What happens if you forget |
130+
|---|---|
131+
| `lib/table/types.ts` `ColumnDefinition` | (this one DOES fail — the ownership loop indexes it) |
132+
| `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type |
133+
| `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved |
134+
| `columns/service.ts` `addTableColumn` param type | callers cannot pass it |
135+
| A metadata-only update path (`updateColumnCurrency` is the model) + a branch in both column routes + the copilot tool | changing it on an existing column is a silent 200 no-op |
136+
| `column-config-sidebar.tsx` | no UI to set it |
137+
| `table-grid.tsx` delete-column undo + `use-table-undo.ts` restore | undo silently resets it to the default |
138+
139+
`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit.
140+
141+
**Known gap:** the metadata-only update path is ~6 near-identical copies (service + 2 routes + copilot). A `metadataUpdate` descriptor on `ColumnTypeServerDefinition` would collapse them; until that exists, copy `currency`'s.
142+
118143
## Checklist Before Finishing
119144

120145
- [ ] Added to the `ColumnType` union in `column-types/types.ts`
@@ -129,7 +154,7 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
129154
## Final Validation (Required)
130155

131156
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/`. 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.
157+
2. **Grep for leaks**`grep -rnE "(===|!==) '{id}'|case '{id}':" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/`. (All three forms: a plain `!==` and a `case` are how half of `currency`'s real branches are written.) 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.
133158
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.
134159
4. **`bun run lint:check`, `bun run check:api-validation`, `bun run check:client-boundary`** from the repo root.
135160
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: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@ Do **not** hunt for places to edit. Add your type to the `ColumnType` union firs
1717
cd apps/sim && bunx tsc --noEmit -p tsconfig.json
1818
```
1919

20-
You will get exactly two errors, naming `column-types/registry.ts` and `column-types/registry.server.ts`. Those are the only two files you must register in. If you get a third error somewhere else, that site is reading a hardcoded type list that should be reading the registry — fix that site, don't work around it.
20+
You will get two errors, naming `column-types/registry.ts` and `column-types/registry.server.ts`. Register in both.
21+
22+
If your type owns metadata, adding its key to `TYPE_SPECIFIC_COLUMN_KEYS` produces two more legitimate errors — `FOREIGN_METADATA_VERB` in `validation.ts` (a `Record` over those keys) and the key's absence from `ColumnDefinition`. Those are the gate working, not sites to "fix".
23+
24+
Any error beyond those four is a site reading a hardcoded type list that should read the registry — fix that site, don't work around it.
2125

2226
## Directory Structure
2327

@@ -91,7 +95,9 @@ The three that are easy to get wrong:
9195

9296
## Step 4: Register
9397

94-
Add the entry to `COLUMN_TYPE_REGISTRY` in `registry.ts` **and** `COLUMN_TYPE_SERVER_REGISTRY` in `registry.server.ts`. `COLUMN_TYPES` derives from the registry keys, so `constants.ts`, the zod contract, and every validator pick it up with no edit.
98+
Add the entry to `COLUMN_TYPE_REGISTRY` in `registry.ts` **and** `COLUMN_TYPE_SERVER_REGISTRY` in `registry.server.ts`.
99+
100+
`COLUMN_TYPES` is declared in `types.ts` (not derived from the registry — the registry is annotated `Record<ColumnType, …>` against it, which is the gate). `constants.ts` re-exports it, so `columnTypeSchema = z.enum(COLUMN_TYPES)` picks your type up with no edit. **Type-specific metadata does not** — see the next step.
95101

96102
## Step 5: Migrations (only if the stored bytes change)
97103

@@ -112,8 +118,27 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
112118
- **Import cycles.** `column-types/select.ts` imports `select-values.ts`, so `select-values.ts` must **not** import the registry — that closes a cycle and fails at module init. Inside a type's own helper module the string literal is the implementation, not a config leak.
113119
- **The client-safe boundary.** `registry.ts` and everything it imports must stay free of `@sim/db`, `drizzle-orm`, and `next/server` — the tables grid imports it directly. A React icon is fine (it's a component *reference*, never called server-side). Only `registry.server.ts` may touch drizzle.
114120
- **Don't re-export the registry from `@/lib/table`.** 44 server modules import that barrel; routing this through it pulls `@sim/emcn/icons` into all of them. Deep-import `@/lib/table/column-types`.
121+
- **`import.ts`'s `coerceValue` is a SECOND write path and is not opt-in.** Importing into a column of your type always hits it, and its `default` arm silently `String(value)`s — so a missing `case` stores text in a column whose `jsonbCast` is numeric, and then every filter and sort on that column errors in Postgres. Add a `case`, even though the switch compiles without one. (It is deliberately separate from the registry's `coerce`: an import wants an unparseable value to survive as its raw string so the row error can name it.)
115122
- **CSV inference** is an ordered heuristic in `import.ts`, deliberately not registry-driven. A new type is not inferred from a CSV unless you extend `inferColumnType` — usually you should not, since inference cannot supply configuration (an option set, a currency code).
116123

124+
## If your type owns metadata, read this
125+
126+
Registering the *type* is compiler-enforced. Registering its *metadata* is not, and that is where the remaining manual work lives. A key like `precision` has to be added in each of these, none of which will fail to compile if you forget:
127+
128+
| Where | What happens if you forget |
129+
|---|---|
130+
| `lib/table/types.ts` `ColumnDefinition` | (this one DOES fail — the ownership loop indexes it) |
131+
| `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type |
132+
| `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved |
133+
| `columns/service.ts` `addTableColumn` param type | callers cannot pass it |
134+
| A metadata-only update path (`updateColumnCurrency` is the model) + a branch in both column routes + the copilot tool | changing it on an existing column is a silent 200 no-op |
135+
| `column-config-sidebar.tsx` | no UI to set it |
136+
| `table-grid.tsx` delete-column undo + `use-table-undo.ts` restore | undo silently resets it to the default |
137+
138+
`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit.
139+
140+
**Known gap:** the metadata-only update path is ~6 near-identical copies (service + 2 routes + copilot). A `metadataUpdate` descriptor on `ColumnTypeServerDefinition` would collapse them; until that exists, copy `currency`'s.
141+
117142
## Checklist Before Finishing
118143

119144
- [ ] Added to the `ColumnType` union in `column-types/types.ts`
@@ -128,7 +153,7 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
128153
## Final Validation (Required)
129154

130155
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/`. 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.
156+
2. **Grep for leaks**`grep -rnE "(===|!==) '{id}'|case '{id}':" apps/sim --include='*.ts' --include='*.tsx' | grep -v column-types/`. (All three forms: a plain `!==` and a `case` are how half of `currency`'s real branches are written.) 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.
132157
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.
133158
4. **`bun run lint:check`, `bun run check:api-validation`, `bun run check:client-boundary`** from the repo root.
134159
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.

0 commit comments

Comments
 (0)