Skip to content

Commit d08eebc

Browse files
committed
docs(skill): update add-column-type for the registry-driven metadata path
Closes the 'Known gap' the skill itself flagged — the ~6 near-identical metadata update paths no longer exist, so the instruction to copy currency's is now wrong. Records the single ownership map, the display hook, the registry-driven import fallback (a new type needs no coerceValue case), and the backward-compatibility trap when adding a key to an existing type.
1 parent be46d36 commit d08eebc

3 files changed

Lines changed: 48 additions & 18 deletions

File tree

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

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,8 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
119119
- **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.
120120
- **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.
121121
- **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.)
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. Its `default` arm now falls back to the registry's own `coerce` and, on failure, keeps the raw string only when `jsonbCast` is `null` — a numeric/timestamptz column nulls instead, because text left in one makes every filter and sort on that column error in Postgres. So a new type needs **no `case`**; add one only when the import should be *more* forgiving than `coerce` (as `date` and `currency` are). Do not restore a bare `String(value)` fallback.
123+
- **Rendering is a registry hook, not a branch.** `cell-render.tsx` reads `definition.display(value, column)` and switches on the returned `ColumnCellDisplay` kind. If your type draws as plain text you implement nothing. Note `display` also owns the *null* decision: a type that renders when its cell is empty (`boolean`'s unchecked box, `select`'s muted "None") must say so there.
123124
- **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).
124125

125126
## If your type owns metadata, read this
@@ -132,13 +133,21 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not,
132133
| `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type |
133134
| `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved |
134135
| `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 |
136136
| `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 |
138137

139-
`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit.
138+
`normalizeColumn`, `buildConvertedColumn`, the undo snapshot, both column routes, the copilot tool, and the metadata-only update path all read the key list generically, so they are zero-edit.
140139

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.
140+
**Declare ownership in ONE place: `METADATA_KEY_OWNERS` in `column-types/types.ts`.** Each type's `ownedMetadata` derives from it via `ownedKeysOf(id)` — do not hand-write an `ownedMetadata` array. The map lives in `types.ts` rather than on the definitions because that module imports no icons, which is what lets `lib/api/contracts/tables.ts` (client-reachable, and so barred from the icon-carrying registry) enforce the same ownership rule the server does. A key may have several owners: `precision` is shared by `number` and `percent`, which is what lets a column convert between them without the key being stripped in transit.
141+
142+
**There is no longer a per-key update path to copy.** `updateColumnMetadata` in `columns/service.ts` handles every key: ownership from `ownedMetadata`, normalization from `defaultMetadata`, validation from `validateDefinition`. Both routes and the copilot tool route on `metadataKeysIn(updates)` and share the `validateMetadataUpdate` pre-flight in `columns/metadata.ts`. Adding a key needs no edit in any of them.
143+
144+
- If changing your key must **rewrite cells**, declare `migrateCellsForMetadata` on the server entry (`date`'s `includeTime` is the model). It runs inside the same transaction with a scaled statement timeout. Omit it for presentational metadata — a `currency` code or a `precision` must never touch a row.
145+
- If your key needs a **dedicated writer** because changing it rewrites cells *and* needs bespoke guards, set `genericMetadataUpdate: []` and keep that writer. Only `select`'s `options`/`multiple` do this.
146+
- **Adding a key to an existing type is a backward-compatibility question.** Absent is not the same as your default: existing columns have no value for it, and treating them as if they chose your default can silently rewrite their data on the next cell write. `date.includeTime` truncates only on an explicit `false`, and stamps `false` on new columns via `defaultMetadata` — so new columns get the good default and old ones are untouched. Assert both directions in a test.
147+
148+
## Deriving UI from the registry
149+
150+
Do not gate a control on a type name (`typeInput === 'number' || typeInput === 'percent'`). Ask `typeOwnsMetadataKey(type, 'precision')`. This is the leak the Step-2 grep below is most likely to catch in your own diff.
142151

143152
## Checklist Before Finishing
144153

@@ -147,7 +156,8 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not,
147156
- [ ] Registered in **both** `registry.ts` and `registry.server.ts`
148157
- [ ] Icon added, centered on the family's optical center, exported alphabetically
149158
- [ ] `migrateCellsTo` / `migrateCellsFrom` added if the stored bytes change
150-
- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `FOREIGN_METADATA_VERB`
159+
- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `METADATA_KEY_OWNERS` + `FOREIGN_METADATA_VERB`, with `ownedMetadata: ownedKeysOf('{id}')`
160+
- [ ] A metadata key added to an EXISTING type leaves columns that predate it behaving exactly as before
151161
- [ ] Unit tests for `coerce` / `isCompatibleWith` round-trips, verified to fail without the code
152162
- [ ] Docs row added to `apps/docs/content/docs/en/tables/index.mdx`
153163

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

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,8 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa
118118
- **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.
119119
- **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.
120120
- **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.)
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. Its `default` arm now falls back to the registry's own `coerce` and, on failure, keeps the raw string only when `jsonbCast` is `null` — a numeric/timestamptz column nulls instead, because text left in one makes every filter and sort on that column error in Postgres. So a new type needs **no `case`**; add one only when the import should be *more* forgiving than `coerce` (as `date` and `currency` are). Do not restore a bare `String(value)` fallback.
122+
- **Rendering is a registry hook, not a branch.** `cell-render.tsx` reads `definition.display(value, column)` and switches on the returned `ColumnCellDisplay` kind. If your type draws as plain text you implement nothing. Note `display` also owns the *null* decision: a type that renders when its cell is empty (`boolean`'s unchecked box, `select`'s muted "None") must say so there.
122123
- **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).
123124

124125
## If your type owns metadata, read this
@@ -131,13 +132,21 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not,
131132
| `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type |
132133
| `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved |
133134
| `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 |
135135
| `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 |
137136

138-
`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit.
137+
`normalizeColumn`, `buildConvertedColumn`, the undo snapshot, both column routes, the copilot tool, and the metadata-only update path all read the key list generically, so they are zero-edit.
139138

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.
139+
**Declare ownership in ONE place: `METADATA_KEY_OWNERS` in `column-types/types.ts`.** Each type's `ownedMetadata` derives from it via `ownedKeysOf(id)` — do not hand-write an `ownedMetadata` array. The map lives in `types.ts` rather than on the definitions because that module imports no icons, which is what lets `lib/api/contracts/tables.ts` (client-reachable, and so barred from the icon-carrying registry) enforce the same ownership rule the server does. A key may have several owners: `precision` is shared by `number` and `percent`, which is what lets a column convert between them without the key being stripped in transit.
140+
141+
**There is no longer a per-key update path to copy.** `updateColumnMetadata` in `columns/service.ts` handles every key: ownership from `ownedMetadata`, normalization from `defaultMetadata`, validation from `validateDefinition`. Both routes and the copilot tool route on `metadataKeysIn(updates)` and share the `validateMetadataUpdate` pre-flight in `columns/metadata.ts`. Adding a key needs no edit in any of them.
142+
143+
- If changing your key must **rewrite cells**, declare `migrateCellsForMetadata` on the server entry (`date`'s `includeTime` is the model). It runs inside the same transaction with a scaled statement timeout. Omit it for presentational metadata — a `currency` code or a `precision` must never touch a row.
144+
- If your key needs a **dedicated writer** because changing it rewrites cells *and* needs bespoke guards, set `genericMetadataUpdate: []` and keep that writer. Only `select`'s `options`/`multiple` do this.
145+
- **Adding a key to an existing type is a backward-compatibility question.** Absent is not the same as your default: existing columns have no value for it, and treating them as if they chose your default can silently rewrite their data on the next cell write. `date.includeTime` truncates only on an explicit `false`, and stamps `false` on new columns via `defaultMetadata` — so new columns get the good default and old ones are untouched. Assert both directions in a test.
146+
147+
## Deriving UI from the registry
148+
149+
Do not gate a control on a type name (`typeInput === 'number' || typeInput === 'percent'`). Ask `typeOwnsMetadataKey(type, 'precision')`. This is the leak the Step-2 grep below is most likely to catch in your own diff.
141150

142151
## Checklist Before Finishing
143152

@@ -146,7 +155,8 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not,
146155
- [ ] Registered in **both** `registry.ts` and `registry.server.ts`
147156
- [ ] Icon added, centered on the family's optical center, exported alphabetically
148157
- [ ] `migrateCellsTo` / `migrateCellsFrom` added if the stored bytes change
149-
- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `FOREIGN_METADATA_VERB`
158+
- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `METADATA_KEY_OWNERS` + `FOREIGN_METADATA_VERB`, with `ownedMetadata: ownedKeysOf('{id}')`
159+
- [ ] A metadata key added to an EXISTING type leaves columns that predate it behaving exactly as before
150160
- [ ] Unit tests for `coerce` / `isCompatibleWith` round-trips, verified to fail without the code
151161
- [ ] Docs row added to `apps/docs/content/docs/en/tables/index.mdx`
152162

0 commit comments

Comments
 (0)