diff --git a/.agents/skills/add-column-type/SKILL.md b/.agents/skills/add-column-type/SKILL.md index 05f9816aa5f..4ec804f7fb3 100644 --- a/.agents/skills/add-column-type/SKILL.md +++ b/.agents/skills/add-column-type/SKILL.md @@ -119,7 +119,8 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa - **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. - **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. - **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`. -- **`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.) +- **`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. +- **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. - **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). ## If your type owns metadata, read this @@ -132,13 +133,21 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, | `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type | | `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved | | `columns/service.ts` `addTableColumn` param type | callers cannot pass it | -| 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 | | `column-config-sidebar.tsx` | no UI to set it | -| `table-grid.tsx` delete-column undo + `use-table-undo.ts` restore | undo silently resets it to the default | -`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit. +`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. -**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. +**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. + +**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. + +- 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. +- 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. +- **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. + +## Deriving UI from the registry + +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. ## Checklist Before Finishing @@ -147,7 +156,8 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, - [ ] Registered in **both** `registry.ts` and `registry.server.ts` - [ ] Icon added, centered on the family's optical center, exported alphabetically - [ ] `migrateCellsTo` / `migrateCellsFrom` added if the stored bytes change -- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `FOREIGN_METADATA_VERB` +- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `METADATA_KEY_OWNERS` + `FOREIGN_METADATA_VERB`, with `ownedMetadata: ownedKeysOf('{id}')` +- [ ] A metadata key added to an EXISTING type leaves columns that predate it behaving exactly as before - [ ] Unit tests for `coerce` / `isCompatibleWith` round-trips, verified to fail without the code - [ ] Docs row added to `apps/docs/content/docs/en/tables/index.mdx` diff --git a/.claude/commands/add-column-type.md b/.claude/commands/add-column-type.md index b390ccc0b98..7627c16e250 100644 --- a/.claude/commands/add-column-type.md +++ b/.claude/commands/add-column-type.md @@ -118,7 +118,8 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa - **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. - **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. - **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`. -- **`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.) +- **`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. +- **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. - **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). ## If your type owns metadata, read this @@ -131,13 +132,21 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, | `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type | | `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved | | `columns/service.ts` `addTableColumn` param type | callers cannot pass it | -| 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 | | `column-config-sidebar.tsx` | no UI to set it | -| `table-grid.tsx` delete-column undo + `use-table-undo.ts` restore | undo silently resets it to the default | -`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit. +`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. -**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. +**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. + +**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. + +- 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. +- 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. +- **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. + +## Deriving UI from the registry + +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. ## Checklist Before Finishing @@ -146,7 +155,8 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, - [ ] Registered in **both** `registry.ts` and `registry.server.ts` - [ ] Icon added, centered on the family's optical center, exported alphabetically - [ ] `migrateCellsTo` / `migrateCellsFrom` added if the stored bytes change -- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `FOREIGN_METADATA_VERB` +- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `METADATA_KEY_OWNERS` + `FOREIGN_METADATA_VERB`, with `ownedMetadata: ownedKeysOf('{id}')` +- [ ] A metadata key added to an EXISTING type leaves columns that predate it behaving exactly as before - [ ] Unit tests for `coerce` / `isCompatibleWith` round-trips, verified to fail without the code - [ ] Docs row added to `apps/docs/content/docs/en/tables/index.mdx` diff --git a/.cursor/commands/add-column-type.md b/.cursor/commands/add-column-type.md index f0be823ab6e..afe0c87bb31 100644 --- a/.cursor/commands/add-column-type.md +++ b/.cursor/commands/add-column-type.md @@ -113,7 +113,8 @@ Prefer set-based SQL. When the transform genuinely needs JS (`currency`'s separa - **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. - **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. - **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`. -- **`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.) +- **`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. +- **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. - **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). ## If your type owns metadata, read this @@ -126,13 +127,21 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, | `column-types/types.ts` `TYPE_SPECIFIC_COLUMN_KEYS` | it is never stripped on conversion, and poisons the target type | | `lib/api/contracts/tables.ts` — the schema slot in all three column schemas, plus `refineColumnOptions` | zod strips it at the boundary; silently never saved | | `columns/service.ts` `addTableColumn` param type | callers cannot pass it | -| 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 | | `column-config-sidebar.tsx` | no UI to set it | -| `table-grid.tsx` delete-column undo + `use-table-undo.ts` restore | undo silently resets it to the default | -`normalizeColumn`, `buildConvertedColumn`, and the undo snapshot read `TYPE_SPECIFIC_COLUMN_KEYS` generically, so those three are already zero-edit. +`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. -**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. +**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. + +**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. + +- 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. +- 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. +- **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. + +## Deriving UI from the registry + +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. ## Checklist Before Finishing @@ -141,7 +150,8 @@ Registering the *type* is compiler-enforced. Registering its *metadata* is not, - [ ] Registered in **both** `registry.ts` and `registry.server.ts` - [ ] Icon added, centered on the family's optical center, exported alphabetically - [ ] `migrateCellsTo` / `migrateCellsFrom` added if the stored bytes change -- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `FOREIGN_METADATA_VERB` +- [ ] New metadata keys added to `TYPE_SPECIFIC_COLUMN_KEYS` + `METADATA_KEY_OWNERS` + `FOREIGN_METADATA_VERB`, with `ownedMetadata: ownedKeysOf('{id}')` +- [ ] A metadata key added to an EXISTING type leaves columns that predate it behaving exactly as before - [ ] Unit tests for `coerce` / `isCompatibleWith` round-trips, verified to fail without the code - [ ] Docs row added to `apps/docs/content/docs/en/tables/index.mdx` diff --git a/apps/docs/content/docs/en/tables/index.mdx b/apps/docs/content/docs/en/tables/index.mdx index 913711ecdfa..65bbb690ffb 100644 --- a/apps/docs/content/docs/en/tables/index.mdx +++ b/apps/docs/content/docs/en/tables/index.mdx @@ -23,14 +23,23 @@ Every column has a type, which decides how its values are stored and validated. | **Text** | A free-form string | `"Acme Corp"` | | **Number** | A numeric value | `42` | | **Currency** | An amount in a currency you pick per column | `$1,234.56` | +| **Percent** | A percentage | `25%` | | **Boolean** | `true` or `false` | `true` | -| **Date** | A date | `2026-03-16` | -| **JSON** | An object or array | `{ "tier": "pro" }` | +| **Date** | A date, with or without a time | `2026-03-16` | | **Select** | One of a fixed set of options, or several | `Pro` | +| **Email** | An email address | `person@example.com` | +| **Phone** | A phone number | `+1 555 123 4567` | +| **JSON** | An object or array | `{ "tier": "pro" }` | Types are enforced as you enter values, so a Number column only takes numbers. -A Currency column stores a plain number and renders it in the currency you choose for that column, so filters, sorts, and exports all see the amount itself. Changing a column's currency relabels it — it does not convert the amounts. +Currency and Percent columns both store a plain number, so filters, sorts, and exports see the amount itself rather than its formatting — `> 50%` is an ordinary numeric comparison. Changing a column's currency relabels it; it does not convert the amounts. Number and Percent columns take a decimal-place setting that changes how values are shown without rounding what is stored. + +Email and Phone columns tidy and check values as you enter them — addresses are lower-cased, phone numbers stripped to their digits — so the same value entered two ways matches. A value that isn't a valid address or number is rejected rather than stored. + +A Date column can carry a time of day or just a calendar date. Turning **Include time** off on a column that already has times will drop them. + +Select options can each be given a color, which is used for the pill shown in the cell. ## Editing a table diff --git a/apps/sim/app/api/table/[tableId]/columns/route.test.ts b/apps/sim/app/api/table/[tableId]/columns/route.test.ts index 4ac282861cd..942e91597c4 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.test.ts @@ -12,12 +12,13 @@ import { hybridAuthMockFns } from '@sim/testing' import { getErrorMessage } from '@sim/utils/errors' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { TableRequestError } from '@/lib/table/errors' const { mockCheckAccess, mockRenameColumn, mockUpdateColumnType, - mockUpdateColumnCurrency, + mockUpdateColumnMetadata, mockUpdateColumnOptions, mockUpdateColumnConstraints, mockAddTableColumn, @@ -26,7 +27,7 @@ const { mockCheckAccess: vi.fn(), mockRenameColumn: vi.fn(), mockUpdateColumnType: vi.fn(), - mockUpdateColumnCurrency: vi.fn(), + mockUpdateColumnMetadata: vi.fn(), mockUpdateColumnOptions: vi.fn(), mockUpdateColumnConstraints: vi.fn(), mockAddTableColumn: vi.fn(), @@ -38,7 +39,7 @@ vi.mock('@/lib/table', () => ({ deleteColumn: mockDeleteColumn, renameColumn: mockRenameColumn, updateColumnConstraints: mockUpdateColumnConstraints, - updateColumnCurrency: mockUpdateColumnCurrency, + updateColumnMetadata: mockUpdateColumnMetadata, updateColumnOptions: mockUpdateColumnOptions, updateColumnType: mockUpdateColumnType, })) @@ -92,7 +93,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { }) // The whole point: the rename must not have been committed. expect(mockRenameColumn).not.toHaveBeenCalled() - expect(mockUpdateColumnCurrency).not.toHaveBeenCalled() + expect(mockUpdateColumnMetadata).not.toHaveBeenCalled() }) it('rejects an unsupported currency code without renaming first', async () => { @@ -107,14 +108,17 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { const response = await patch({ name: 'renamed', currencyCode: 'ZZZ' }) expect(response.status).toBe(400) + // The currency type's own `validateDefinition` message — the same one the + // add-column path already returned. The route no longer carries a second, + // differently-worded copy of this check. expect(await response.json()).toMatchObject({ - error: expect.stringContaining('Invalid currency code'), + error: 'Column "amount" has invalid currency code "ZZZ". Use an ISO 4217 code, e.g. USD', }) expect(mockRenameColumn).not.toHaveBeenCalled() }) it('still applies a rename when the currency it rides on is unchanged', async () => { - // `updateColumnCurrency` no-ops on an unchanged code. The rename folded into + // `updateColumnMetadata` no-ops on an unchanged code. The rename folded into // the same request must not be dropped with it. mockCheckAccess.mockResolvedValue({ ok: true, @@ -125,13 +129,13 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { }, }, }) - mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } }) + mockUpdateColumnMetadata.mockResolvedValue({ schema: { columns: [] } }) const response = await patch({ name: 'renamed', currencyCode: 'USD' }) expect(response.status).toBe(200) - expect(mockUpdateColumnCurrency).toHaveBeenCalledWith( - expect.objectContaining({ currencyCode: 'USD', newName: 'renamed' }), + expect(mockUpdateColumnMetadata).toHaveBeenCalledWith( + expect.objectContaining({ metadata: { currencyCode: 'USD' }, newName: 'renamed' }), expect.any(String) ) }) @@ -157,9 +161,12 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { }, }) // Stands in for the race the guards cannot close: the column stopped being - // a currency between the snapshot the guards read and this write. - mockUpdateColumnCurrency.mockRejectedValue( - new Error('Cannot set currency on column "amount" of type "string"') + // a currency between the snapshot the guards read and this write. The + // service raises `TableRequestError` for a caller-fixable failure, which is + // what earns the 400 — a plain `Error` now means the server genuinely broke + // and correctly returns 500. + mockUpdateColumnMetadata.mockRejectedValue( + new TableRequestError('Cannot set currency on column "amount" of type "string"') ) const response = await patch({ name: 'renamed', currencyCode: 'USD' }) @@ -189,7 +196,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { error: expect.stringContaining('already exists'), }) // The typed write would otherwise have committed under a rename that fails. - expect(mockUpdateColumnCurrency).not.toHaveBeenCalled() + expect(mockUpdateColumnMetadata).not.toHaveBeenCalled() expect(mockRenameColumn).not.toHaveBeenCalled() }) @@ -303,7 +310,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { schema: { columns: [{ id: 'col_a', name: 'amount', type: 'currency' }] }, }, }) - mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } }) + mockUpdateColumnMetadata.mockResolvedValue({ schema: { columns: [] } }) const response = await patch({ name: 'renamed', currencyCode: 'eur' }) @@ -311,9 +318,13 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { // One transaction, not two: the rename rides along with the currency write, // so neither half can commit without the other. expect(mockRenameColumn).not.toHaveBeenCalled() - expect(mockUpdateColumnCurrency).toHaveBeenCalledWith( + expect(mockUpdateColumnMetadata).toHaveBeenCalledWith( // Addressed by stable id; the contract upper-cases the code on the way in. - expect.objectContaining({ columnName: 'col_a', currencyCode: 'EUR', newName: 'renamed' }), + expect.objectContaining({ + columnName: 'col_a', + metadata: { currencyCode: 'EUR' }, + newName: 'renamed', + }), expect.any(String) ) }) diff --git a/apps/sim/app/api/table/[tableId]/columns/route.ts b/apps/sim/app/api/table/[tableId]/columns/route.ts index abda376ede7..c03e3216b65 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.ts @@ -15,19 +15,19 @@ import { deleteColumn, renameColumn, updateColumnConstraints, - updateColumnCurrency, + updateColumnMetadata, updateColumnOptions, updateColumnType, } from '@/lib/table' import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys' -import { columnTypeById } from '@/lib/table/column-types' -import { isSupportedCurrencyCode } from '@/lib/table/currency' +import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types' +import { validateMetadataUpdate } from '@/lib/table/columns/metadata' +import { TableRequestError } from '@/lib/table/errors' import { signalTableSchemaChanged } from '@/lib/table/events' import { accessError, checkAccess, normalizeColumn, - rootErrorMessage, tableLockErrorResponse, } from '@/app/api/table/utils' @@ -78,18 +78,10 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Colum return validationErrorResponse(error, 'Invalid request data') } - const msg = rootErrorMessage(error) - if ( - msg.includes('already exists') || - msg.includes('maximum column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('option') - ) { - return NextResponse.json({ error: msg }, { status: 400 }) - } - if (msg === 'Table not found') { - return NextResponse.json({ error: msg }, { status: 404 }) + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error adding column to table ${tableId}:`, error) @@ -145,14 +137,17 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu ) } + // Which type-specific keys this payload carries, and which writer owns + // each. Read from the registry rather than named here, so a new metadata + // key routes correctly without touching this route. + const { generic: genericMetadataKeys, dedicated: dedicatedMetadataKeys } = + metadataKeysIn(updates) + // A retype applies and validates the constraints itself, so the separate // constraint write only runs when the type is unchanged. The rename rides // whichever write actually runs last. const typedWriteRuns = - typeChanging || - updates.currencyCode !== undefined || - updates.options !== undefined || - updates.multiple !== undefined + typeChanging || genericMetadataKeys.length > 0 || dedicatedMetadataKeys.length > 0 const constraintsWriteRuns = !typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined) const renameWithTypedWrite = @@ -165,23 +160,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu // changing: an options-only update on an existing select column carries the // same hazard as a conversion does. const resultingType = updates.type ?? currentColumn?.type - if (updates.currencyCode !== undefined) { - if (resultingType !== 'currency') { - return NextResponse.json( - { - error: `Cannot set currency on column "${validated.columnName}" of type "${resultingType}"`, - }, - { status: 400 } - ) - } - if (!isSupportedCurrencyCode(updates.currencyCode)) { - return NextResponse.json( - { - error: `Invalid currency code "${updates.currencyCode}". Use an ISO 4217 code, e.g. USD`, - }, - { status: 400 } - ) - } + const metadataError = validateMetadataUpdate(currentColumn, resultingType, updates) + if (metadataError) { + return NextResponse.json({ error: metadataError }, { status: 400 }) } // The rename runs last (see below), so a name already taken would fail after // the typed write committed. This is the only rename failure a caller can @@ -224,9 +205,12 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu tableId, columnName: columnRef, newType: updates.type as NonNullable, - ...(updates.options !== undefined ? { options: updates.options } : {}), - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - ...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}), + // Every type-specific key the payload carries, whichever writer would + // own it standalone: a conversion applies its target's metadata in the + // same transaction rather than leaving it to a second write. Clears + // are forwarded as `null` — stripping them here made + // `buildConvertedColumn` fall back to the pre-conversion value. + ...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]), // Forwarded so the conversion validates against the constraint this // same request is about to set, not the column's current one. ...(updates.required !== undefined ? { required: updates.required } : {}), @@ -235,22 +219,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu }, requestId ) - } else if (updates.currencyCode !== undefined) { - // Re-denominating an existing currency column: schema-only, no cell - // rewrite. Reached only when the type is unchanged — a conversion INTO - // currency carries the code through `updateColumnType` above. - updatedTable = await updateColumnCurrency( + } else if (genericMetadataKeys.length > 0) { + // Changing a column's own metadata — re-denominating a currency, changing + // a number's precision. Usually schema-only; the type declares a cell + // rewrite if it needs one. Reached only when the type is unchanged, since + // a conversion carries its metadata through `updateColumnType` above. + updatedTable = await updateColumnMetadata( { tableId, columnName: columnRef, - currencyCode: updates.currencyCode, + metadata: pickMetadata(updates, genericMetadataKeys), ...(updates.required !== undefined ? { required: updates.required } : {}), ...(updates.unique !== undefined ? { unique: updates.unique } : {}), ...renameWithTypedWrite, }, requestId ) - } else if (updates.options !== undefined || updates.multiple !== undefined) { + } else if (dedicatedMetadataKeys.length > 0) { updatedTable = await updateColumnOptions( { tableId, @@ -312,24 +297,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu return validationErrorResponse(error, 'Invalid request data') } - const msg = rootErrorMessage(error) - if (msg.includes('not found') || msg.includes('Table not found')) { - return NextResponse.json({ error: msg }, { status: 404 }) - } - if ( - msg.includes('already exists') || - msg.includes('Cannot delete the last column') || - msg.includes('Cannot set column') || - msg.includes('Cannot set unique column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('incompatible') || - msg.includes('duplicate') || - msg.includes('option') || - msg.includes('currency') || - msg.includes('is already type') - ) { - return NextResponse.json({ error: msg }, { status: 400 }) + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error updating column in table ${tableId}:`, error) @@ -382,12 +353,10 @@ export const DELETE = withRouteHandler( return validationErrorResponse(error, 'Invalid request data') } - const msg = rootErrorMessage(error) - if (msg.includes('not found') || msg === 'Table not found') { - return NextResponse.json({ error: msg }, { status: 404 }) - } - if (msg.includes('Cannot delete') || msg.includes('last column')) { - return NextResponse.json({ error: msg }, { status: 400 }) + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error deleting column from table ${tableId}:`, error) diff --git a/apps/sim/app/api/table/[tableId]/import/route.test.ts b/apps/sim/app/api/table/[tableId]/import/route.test.ts index baf8c313a4f..4e5a44dea41 100644 --- a/apps/sim/app/api/table/[tableId]/import/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/import/route.test.ts @@ -5,6 +5,7 @@ import { hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { TableDefinition } from '@/lib/table' +import { TableRequestError } from '@/lib/table/errors' const { mockCheckAccess, @@ -32,6 +33,7 @@ vi.mock('@sim/utils/id', () => ({ vi.mock('@/app/api/table/utils', async () => { const { NextResponse } = await import('next/server') const { TableLockedError } = await import('@/lib/table/mutation-locks') + const { TableRequestError } = await import('@/lib/table/errors') return { checkAccess: mockCheckAccess, accessError: (result: { status: number }) => { @@ -43,6 +45,10 @@ vi.mock('@/app/api/table/utils', async () => { error instanceof TableLockedError ? NextResponse.json({ error: error.message, lock: error.lock }, { status: 423 }) : null, + tableRequestErrorResponse: (error: unknown) => + error instanceof TableRequestError + ? NextResponse.json({ error: error.message }, { status: error.status }) + : null, multipartErrorResponse: (error: { code: string; message: string }) => NextResponse.json( { error: error.message }, @@ -316,6 +322,43 @@ describe('POST /api/table/[tableId]/import', () => { expect(mockImportAppendRows).not.toHaveBeenCalled() }) + /** + * `addTableColumnsWithTx` runs INSIDE `importAppendRows`, so the service's own + * typed failures surface in the append catch — which returns instead of + * rethrowing, so nothing the outer catch does applies. Without the explicit + * mapping there, the column cap and an invalid column type both come back as + * a 500 whose real reason has been replaced by 'Failed to import CSV'. + */ + it.each([ + ['the column cap', 'Adding 2 column(s) would exceed maximum column limit (100)'], + ['an invalid column type', 'Invalid column type "sometype". Must be one of: string, number'], + ])('surfaces %s from an append as the service status', async (_label, message) => { + mockImportAppendRows.mockRejectedValueOnce(new TableRequestError(message)) + const response = await callPost( + createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' }) + ) + expect(response.status).toBe(400) + expect((await response.json()).error).toBe(message) + }) + + it('keeps a 404 from an append a 404 rather than flattening it to 400', async () => { + mockImportAppendRows.mockRejectedValueOnce(new TableRequestError('Table not found', 404)) + const response = await callPost( + createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' }) + ) + expect(response.status).toBe(404) + expect((await response.json()).error).toBe('Table not found') + }) + + it('still returns a generic 500 for an append failure the service did not type', async () => { + mockImportAppendRows.mockRejectedValueOnce(new Error('connection terminated unexpectedly')) + const response = await callPost( + createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' }) + ) + expect(response.status).toBe(500) + expect((await response.json()).error).toBe('Failed to import CSV') + }) + it('replaces rows via importReplaceRows', async () => { mockImportReplaceRows.mockResolvedValueOnce({ deletedCount: 5, insertedCount: 2 }) const response = await callPost( @@ -428,6 +471,8 @@ describe('POST /api/table/[tableId]/import', () => { ) expect(response.status).toBe(200) expect(mockImportAppendRows).toHaveBeenCalledTimes(1) + // Inferred as `email`, not `string`: the fixture values are real + // addresses, and CSV inference now recognises them. expect(appendAdditions()).toEqual([ expect.objectContaining({ name: 'email', type: 'string' }), ]) @@ -537,6 +582,8 @@ describe('POST /api/table/[tableId]/import', () => { }) ) // Route forwarded the column addition into the (now atomic) import op. + // Inferred as `email`, not `string`: the fixture values are real + // addresses, and CSV inference now recognises them. expect(appendAdditions()).toEqual([ expect.objectContaining({ name: 'email', type: 'string' }), ]) diff --git a/apps/sim/app/api/table/[tableId]/import/route.ts b/apps/sim/app/api/table/[tableId]/import/route.ts index 8ee9ed8f170..946baf30292 100644 --- a/apps/sim/app/api/table/[tableId]/import/route.ts +++ b/apps/sim/app/api/table/[tableId]/import/route.ts @@ -27,7 +27,7 @@ import { dispatchAfterBatchInsert, generateColumnId, getMaxRowsPerTable, - inferColumnType, + inferredColumnDefinition, markTableJobRunning, releaseJobClaim, sanitizeName, @@ -46,6 +46,7 @@ import { csvProxyBodyCapResponse, multipartErrorResponse, tableLockErrorResponse, + tableRequestErrorResponse, } from '@/app/api/table/utils' const logger = createLogger('TableImportCSVExisting') @@ -229,15 +230,21 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro suffix++ } usedNames.add(columnName.toLowerCase()) - const inferredType = inferColumnType(rows.map((r) => r[header])) + // Same helper the create-from-CSV path uses, so the two cannot answer + // differently. Inlining just the TYPE here persisted an appended date + // column as date-only while its rows were coerced with their times. + const inferred = inferredColumnDefinition( + columnName, + rows.map((r) => r[header]) + ) // Pre-assign the id so the prospective schema (used to coerce rows) and // the persisted column (created in importAppendRows) share the same key. const id = generateColumnId() - additions.push({ id, name: columnName, type: inferredType }) + additions.push({ ...inferred, id }) newColumns.push({ + ...inferred, id, - name: columnName, - type: inferredType as TableSchema['columns'][number]['type'], + type: inferred.type as TableSchema['columns'][number]['type'], required: false, unique: false, }) @@ -339,11 +346,17 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro }, }) } catch (err) { - // This branch returns rather than rethrowing, so the outer catch's - // mapper is unreachable from here — map the lock error first or a 423 - // degrades into a generic 500 (replace mode rethrows and maps fine). + // This branch returns rather than rethrowing, so NOTHING in the outer + // catch runs for an append failure — every mapper it applies has to be + // repeated here (replace mode rethrows and maps fine). A 423 lock + // violation and the service's own typed failures both degrade into a + // generic 500 without these two lines: `addTableColumnsWithTx` runs + // INSIDE `importAppendRows`, so an invalid column name or the column + // cap surfaces here, not out there. const lockError = tableLockErrorResponse(err) if (lockError) return lockError + const requestError = tableRequestErrorResponse(err) + if (requestError) return requestError const message = toError(err).message logger.warn(`[${requestId}] Append failed for table ${tableId}`, { @@ -425,6 +438,14 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro const message = toError(error).message logger.error(`[${requestId}] CSV import into existing table failed:`, error) + // The table service says whether a failure is the caller's and what status + // it deserves. The substring list below still covers the CSV parser, which + // is not table-aware — but the service's own validation (an invalid column + // type, the column cap) matched none of those strings and was reported as a + // 500 with the message swallowed. + const requestError = tableRequestErrorResponse(error) + if (requestError) return requestError + const isClientError = message.includes('CSV file has no') || message.includes('already exists') || diff --git a/apps/sim/app/api/table/import-csv/route.test.ts b/apps/sim/app/api/table/import-csv/route.test.ts index b85e1ccb01b..c51921a47ba 100644 --- a/apps/sim/app/api/table/import-csv/route.test.ts +++ b/apps/sim/app/api/table/import-csv/route.test.ts @@ -32,6 +32,7 @@ vi.mock('@/lib/table/rows/service', () => ({ vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetLimits })) vi.mock('@/app/api/table/utils', async () => { const { NextResponse } = await import('next/server') + const { TableRequestError } = await import('@/lib/table/errors') return { normalizeColumn: (column: unknown) => column, csvProxyBodyCapResponse: () => null, @@ -40,6 +41,10 @@ vi.mock('@/app/api/table/utils', async () => { { error: error.message }, { status: error.code === 'FILE_TOO_LARGE' ? 413 : 400 } ), + tableRequestErrorResponse: (error: unknown) => + error instanceof TableRequestError + ? NextResponse.json({ error: error.message }, { status: error.status }) + : null, rowWriteErrorResponse: (error: unknown) => { const message = getErrorMessage(error) return message.includes('row limit') @@ -50,6 +55,7 @@ vi.mock('@/app/api/table/utils', async () => { }) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) +import { TableRequestError } from '@/lib/table/errors' import { POST } from '@/app/api/table/import-csv/route' type Part = @@ -193,6 +199,20 @@ describe('POST /api/table/import-csv', () => { expect(data.error).toMatch(/row limit/) }) + /** + * `createTable` validates the name, the schema, the per-column rules, and both + * plan caps. The substring list in this route's catch named only some of those + * messages, so the rest reached the client as a generic 500. + */ + it('surfaces a typed createTable failure the substring list never named', async () => { + const message = 'Column name exceeds maximum length (64 characters)' + mockCreateTable.mockRejectedValueOnce(new TableRequestError(message)) + const response = await POST(makeRequest(uploadParts(csvWithRows(5)))) + + expect(response.status).toBe(400) + expect((await response.json()).error).toBe(message) + }) + it('rolls back the created table when a batch insert fails mid-stream', async () => { mockBatchInsertRows .mockResolvedValueOnce(Array.from({ length: 100 }, () => ({ id: 'row' }))) diff --git a/apps/sim/app/api/table/import-csv/route.ts b/apps/sim/app/api/table/import-csv/route.ts index 9ca0381fe90..30bbb2dc4c0 100644 --- a/apps/sim/app/api/table/import-csv/route.ts +++ b/apps/sim/app/api/table/import-csv/route.ts @@ -35,6 +35,7 @@ import { multipartErrorResponse, normalizeColumn, rowWriteErrorResponse, + tableRequestErrorResponse, } from '@/app/api/table/utils' const logger = createLogger('TableImportCSV') @@ -254,6 +255,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => { const rowWriteError = rowWriteErrorResponse(error) if (rowWriteError) return rowWriteError + // `createTable` validates the name, the schema, the per-column rules, and + // both plan caps — most of which the substring list below never named. + const requestError = tableRequestErrorResponse(error) + if (requestError) return requestError + const message = toError(error).message const isClientError = message.includes('maximum table limit') || diff --git a/apps/sim/app/api/table/route.ts b/apps/sim/app/api/table/route.ts index 2522cddb7c6..5c09c13d6fd 100644 --- a/apps/sim/app/api/table/route.ts +++ b/apps/sim/app/api/table/route.ts @@ -15,6 +15,7 @@ import { type TableSchema, type TableScope, } from '@/lib/table' +import { TableRequestError } from '@/lib/table/errors' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { normalizeColumn } from '@/app/api/table/utils' @@ -154,6 +155,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => { }) } catch (error) { if (error instanceof Error) { + // One typed check: the service says whether a failure is the caller's + // and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) + } if (error.message.includes('maximum table limit')) { return NextResponse.json({ error: error.message }, { status: 403 }) } diff --git a/apps/sim/app/api/table/utils.test.ts b/apps/sim/app/api/table/utils.test.ts index 99d0ce0c5a5..61de0936ee8 100644 --- a/apps/sim/app/api/table/utils.test.ts +++ b/apps/sim/app/api/table/utils.test.ts @@ -3,8 +3,14 @@ */ import { describe, expect, it } from 'vitest' import { TableRowLimitError } from '@/lib/table/billing' +import { TableRequestError } from '@/lib/table/errors' import type { ColumnDefinition } from '@/lib/table/types' -import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils' +import { + rootErrorMessage, + rowWriteErrorResponse, + tableFilterError, + tableRequestErrorResponse, +} from '@/app/api/table/utils' /** Mimics drizzle's DrizzleQueryError: message is the failed SQL, real error on `cause`. */ function wrapLikeDrizzle(cause: Error): Error { @@ -55,6 +61,34 @@ describe('rowWriteErrorResponse', () => { }) }) +/** + * The service classifies its own failures, so callers must not re-derive the + * verdict from message text. These cases are exactly the ones no substring list + * named — which is how they reached clients as a generic 500. + */ +describe('tableRequestErrorResponse', () => { + it('carries the service message at the status the service chose', async () => { + const response = tableRequestErrorResponse( + new TableRequestError('Adding 2 column(s) would exceed maximum column limit (100)') + ) + expect(response?.status).toBe(400) + const body = await response?.json() + expect(body.error).toBe('Adding 2 column(s) would exceed maximum column limit (100)') + }) + + it('preserves a 404 rather than flattening every typed failure to 400', () => { + expect(tableRequestErrorResponse(new TableRequestError('Table not found', 404))?.status).toBe( + 404 + ) + }) + + it('returns null for anything the service did not type', () => { + expect(tableRequestErrorResponse(new Error('connection refused'))).toBeNull() + expect(tableRequestErrorResponse(new TableRowLimitError(10000))).toBeNull() + expect(tableRequestErrorResponse('not an error')).toBeNull() + }) +}) + /** * The async destructive routes (delete-async, cancel-runs, columns/run) * validate the WIRE filter here. The predicate branch must reject unknown diff --git a/apps/sim/app/api/table/utils.ts b/apps/sim/app/api/table/utils.ts index ceb399556c4..4928f3ce295 100644 --- a/apps/sim/app/api/table/utils.ts +++ b/apps/sim/app/api/table/utils.ts @@ -13,6 +13,7 @@ import type { ColumnDefinition, Filter, TableDefinition, TablePredicate } from ' import { buildFilterClause, getTableById, TableQueryValidationError } from '@/lib/table' import { typeMetadataOf } from '@/lib/table/column-types' import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants' +import { TableRequestError } from '@/lib/table/errors' import { TableLockedError } from '@/lib/table/mutation-locks' import { isTablePredicate } from '@/lib/table/query-builder/converters' import { validateStoragePredicate } from '@/lib/table/query-builder/validate' @@ -105,6 +106,22 @@ export function rootErrorMessage(error: unknown): string { return toError(current).message } +/** + * Maps the table service's own typed failures to the status they declare, or + * `null` when the error came from somewhere else. + * + * Prefer this over matching message substrings: the service says whose fault a + * failure is, so a new validation message is classified correctly the day it is + * added. The substring lists still cover the layers that are not table-aware + * (the CSV parser, drizzle), which is why callers run both — this one first, + * since a typed error carries an explicit status that a substring match would + * flatten to 400. + */ +export function tableRequestErrorResponse(error: unknown): NextResponse | null { + if (!(error instanceof TableRequestError)) return null + return NextResponse.json({ error: error.message }, { status: error.status }) +} + /** * Known user-facing row-write failures (service validation + the best-effort * plan row-limit check). Anything outside this list stays a generic 500 — diff --git a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts index b5471842a6c..76d3ebf0453 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts @@ -14,13 +14,14 @@ import { deleteColumn, renameColumn, updateColumnConstraints, - updateColumnCurrency, + updateColumnMetadata, updateColumnOptions, updateColumnType, } from '@/lib/table' import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys' -import { columnTypeById } from '@/lib/table/column-types' -import { isSupportedCurrencyCode } from '@/lib/table/currency' +import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types' +import { validateMetadataUpdate } from '@/lib/table/columns/metadata' +import { TableRequestError } from '@/lib/table/errors' import { signalTableSchemaChanged } from '@/lib/table/events' import { accessError, @@ -103,21 +104,10 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Colum const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - // Same caller-error set the internal columns route maps — an invalid - // select option set is a bad request, not a server fault. - if ( - error.message.includes('already exists') || - error.message.includes('maximum column') || - error.message.includes('Invalid column') || - error.message.includes('exceeds maximum') || - error.message.includes('option') - ) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } - if (error.message === 'Table not found') { - return NextResponse.json({ error: error.message }, { status: 404 }) - } + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error adding column to table:`, error) @@ -179,14 +169,17 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu ) } + // Which type-specific keys this payload carries, and which writer owns + // each. Read from the registry rather than named here, so a new metadata + // key routes correctly without touching this route. + const { generic: genericMetadataKeys, dedicated: dedicatedMetadataKeys } = + metadataKeysIn(updates) + // A retype applies and validates the constraints itself, so the separate // constraint write only runs when the type is unchanged. The rename rides // whichever write actually runs last. const typedWriteRuns = - typeChanging || - updates.currencyCode !== undefined || - updates.options !== undefined || - updates.multiple !== undefined + typeChanging || genericMetadataKeys.length > 0 || dedicatedMetadataKeys.length > 0 const constraintsWriteRuns = !typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined) const renameWithTypedWrite = @@ -199,23 +192,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu // changing: an options-only update on an existing select column carries the // same hazard as a conversion does. const resultingType = updates.type ?? currentColumn?.type - if (updates.currencyCode !== undefined) { - if (resultingType !== 'currency') { - return NextResponse.json( - { - error: `Cannot set currency on column "${validated.columnName}" of type "${resultingType}"`, - }, - { status: 400 } - ) - } - if (!isSupportedCurrencyCode(updates.currencyCode)) { - return NextResponse.json( - { - error: `Invalid currency code "${updates.currencyCode}". Use an ISO 4217 code, e.g. USD`, - }, - { status: 400 } - ) - } + const metadataError = validateMetadataUpdate(currentColumn, resultingType, updates) + if (metadataError) { + return NextResponse.json({ error: metadataError }, { status: 400 }) } // The rename runs last (see below), so a name already taken would fail after // the typed write committed. This is the only rename failure a caller can @@ -258,9 +237,12 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu tableId, columnName: columnRef, newType: updates.type as NonNullable, - ...(updates.options !== undefined ? { options: updates.options } : {}), - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - ...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}), + // Every type-specific key the payload carries, whichever writer would + // own it standalone: a conversion applies its target's metadata in the + // same transaction rather than leaving it to a second write. Clears + // are forwarded as `null` — stripping them here made + // `buildConvertedColumn` fall back to the pre-conversion value. + ...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]), // Forwarded so the conversion validates against the constraint this // same request is about to set, not the column's current one. ...(updates.required !== undefined ? { required: updates.required } : {}), @@ -269,22 +251,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu }, requestId ) - } else if (updates.currencyCode !== undefined) { - // Re-denominating an existing currency column: schema-only, no cell - // rewrite. Reached only when the type is unchanged — a conversion INTO - // currency carries the code through `updateColumnType` above. - updatedTable = await updateColumnCurrency( + } else if (genericMetadataKeys.length > 0) { + // Changing a column's own metadata — re-denominating a currency, changing + // a number's precision. Usually schema-only; the type declares a cell + // rewrite if it needs one. Reached only when the type is unchanged, since + // a conversion carries its metadata through `updateColumnType` above. + updatedTable = await updateColumnMetadata( { tableId, columnName: columnRef, - currencyCode: updates.currencyCode, + metadata: pickMetadata(updates, genericMetadataKeys), ...(updates.required !== undefined ? { required: updates.required } : {}), ...(updates.unique !== undefined ? { unique: updates.unique } : {}), ...renameWithTypedWrite, }, requestId ) - } else if (updates.options !== undefined || updates.multiple !== undefined) { + } else if (dedicatedMetadataKeys.length > 0) { updatedTable = await updateColumnOptions( { tableId, @@ -357,25 +340,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - const msg = error.message - if (msg.includes('not found') || msg.includes('Table not found')) { - return NextResponse.json({ error: msg }, { status: 404 }) - } - if ( - msg.includes('already exists') || - msg.includes('Cannot delete the last column') || - msg.includes('Cannot set column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('incompatible') || - msg.includes('duplicate') || - msg.includes('option') || - msg.includes('currency') || - msg.includes('is already type') - ) { - return NextResponse.json({ error: msg }, { status: 400 }) - } + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error updating column in table:`, error) @@ -445,13 +413,10 @@ export const DELETE = withRouteHandler( const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - if (error.message.includes('not found') || error.message === 'Table not found') { - return NextResponse.json({ error: error.message }, { status: 404 }) - } - if (error.message.includes('Cannot delete') || error.message.includes('last column')) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } + // One typed check instead of a per-message substring list: the service + // says whether a failure is the caller's and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) } logger.error(`[${requestId}] Error deleting column from table:`, error) diff --git a/apps/sim/app/api/v1/tables/route.ts b/apps/sim/app/api/v1/tables/route.ts index 82bc6618247..cabedd07116 100644 --- a/apps/sim/app/api/v1/tables/route.ts +++ b/apps/sim/app/api/v1/tables/route.ts @@ -6,6 +6,7 @@ import { parseRequest } from '@/lib/api/server' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { createTable, getWorkspaceTableLimits, listTables, type TableSchema } from '@/lib/table' +import { TableRequestError } from '@/lib/table/errors' import { normalizeColumn } from '@/app/api/table/utils' import { checkRateLimit, @@ -172,6 +173,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => { if (validationResponse) return validationResponse if (error instanceof Error) { + // One typed check: the service says whether a failure is the caller's + // and what status it deserves. + if (error instanceof TableRequestError) { + return NextResponse.json({ error: error.message }, { status: error.status }) + } if (error.message.includes('maximum table limit')) { return NextResponse.json({ error: error.message }, { status: 403 }) } diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx index 116b43de286..3818e0966d9 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx @@ -6,11 +6,13 @@ import { X } from '@sim/emcn/icons' import { toError } from '@sim/utils/errors' import { findValidationIssue, isValidationError } from '@/lib/api/client/errors' import type { ColumnDefinition, SelectOption } from '@/lib/table' +import { typeOwnsMetadataKey } from '@/lib/table/column-types' import { DEFAULT_CURRENCY_CODE, getCurrencyOptions, resolveCurrencyCode, } from '@/lib/table/currency' +import { clampPrecision, DEFAULT_PRECISION } from '@/lib/table/precision' import { FieldError, RequiredLabel, @@ -21,7 +23,7 @@ import { PLAIN_COLUMN_TYPE_OPTIONS } from './column-types' /** Whether a column type carries an option set. */ function isSelectType(type: ColumnDefinition['type']): boolean { - return type === 'select' + return typeOwnsMetadataKey(type, 'options') } /** @@ -115,6 +117,22 @@ function ColumnConfigBody({ const [typeInput, setTypeInput] = useState(() => config.mode === 'edit' ? (existingColumn?.type ?? 'string') : config.type ) + // What "include time" means for the column as it stands today. + // + // Only an EXISTING date column answers from its own value, where absent means + // a column predating the key and therefore holding instants. Every other + // starting point — creating a column, or converting one that is not yet a + // date — takes the same date-only default a newly created date column gets. + // + // Read off a non-date column, `includeTime !== false` answers `true` (the key + // is simply absent there), which made converting a text column to Date + // silently opt it into times while creating one gave date-only. The seed and + // the dirty-check both read this so they cannot answer differently. + const baselineIncludeTime = + config.mode === 'edit' && existingColumn?.type === 'date' + ? existingColumn.includeTime !== false + : false + const [uniqueInput, setUniqueInput] = useState(() => config.mode === 'edit' ? !!existingColumn?.unique : false ) @@ -129,6 +147,18 @@ function ColumnConfigBody({ ? resolveCurrencyCode(existingColumn?.currencyCode) : DEFAULT_CURRENCY_CODE ) + // The RAW string, not a number. A numeric state round-tripped through + // `Number()` on every keystroke cannot be cleared (`'' → 0`) and clamps + // mid-typing (`1`, then `2` → `10`, never `12`). The parse is derived below + // and never written back, so the field keeps whatever was typed — same shape + // as `usage-limit-field`. Empty means "no precision declared", which is what + // keeps a column rendering its values as stored. + const [precisionInput, setPrecisionInput] = useState(() => + config.mode === 'edit' && existingColumn?.precision !== undefined + ? String(existingColumn.precision) + : '' + ) + const [includeTimeInput, setIncludeTimeInput] = useState(() => baselineIncludeTime) const [showValidation, setShowValidation] = useState(false) const [nameError, setNameError] = useState(null) const [optionsError, setOptionsError] = useState(null) @@ -136,7 +166,22 @@ function ColumnConfigBody({ const saveDisabled = updateColumn.isPending || addColumn.isPending const trimmedName = nameInput.trim() const wantsOptions = isSelectType(typeInput) - const wantsCurrency = typeInput === 'currency' + // Which metadata controls to show is a registry question, not a list of type + // names: a type that later gains `precision` gets the control for free, and a + // type that loses it cannot leave a stale control behind. + const wantsCurrency = typeOwnsMetadataKey(typeInput, 'currencyCode') + const wantsPrecision = typeOwnsMetadataKey(typeInput, 'precision') + // `undefined` means "no precision declared" — the field is legitimately + // clearable back to rendering values as stored. Anything that is not a whole + // number is treated the same rather than clamped, because `clampPrecision` + // falls back to 0 for a non-integer: `2.5` is finite, so a `Number.isFinite` + // guard let it through and silently saved "zero decimal places". + const precisionNumber = Number(precisionInput) + const parsedPrecision = + precisionInput.trim() === '' || !Number.isInteger(precisionNumber) + ? undefined + : clampPrecision(precisionNumber) + const wantsIncludeTime = typeOwnsMetadataKey(typeInput, 'includeTime') const trimmedOptions = optionsInput.map((o) => ({ ...o, name: o.name.trim() })) /** Client-side option validation mirroring the server rules; returns an error message or null. */ @@ -171,6 +216,10 @@ function ColumnConfigBody({ ...(wantsOptions ? { options: trimmedOptions } : {}), ...(wantsOptions && multipleInput ? { multiple: true } : {}), ...(wantsCurrency ? { currencyCode: currencyInput } : {}), + ...(wantsPrecision && parsedPrecision !== undefined + ? { precision: parsedPrecision } + : {}), + ...(wantsIncludeTime ? { includeTime: includeTimeInput } : {}), }) toast.success(`Added "${trimmedName}"`) onClose() @@ -191,6 +240,8 @@ function ColumnConfigBody({ const multipleChanged = wantsOptions && !!existingColumn?.multiple !== multipleInput const currencyChanged = wantsCurrency && resolveCurrencyCode(existingColumn?.currencyCode) !== currencyInput + const precisionChanged = wantsPrecision && existingColumn?.precision !== parsedPrecision + const includeTimeChanged = wantsIncludeTime && baselineIncludeTime !== includeTimeInput const updates: { name?: string @@ -199,6 +250,8 @@ function ColumnConfigBody({ options?: SelectOption[] multiple?: boolean currencyCode?: string + precision?: number | null + includeTime?: boolean } = { ...(renamed ? { name: trimmedName } : {}), ...(typeChanged ? { type: typeInput } : {}), @@ -209,6 +262,15 @@ function ColumnConfigBody({ ...(wantsCurrency && (typeChanged || currencyChanged) ? { currencyCode: currencyInput } : {}), + // `null` clears the key. Gating on `!== undefined` meant emptying the + // field sent nothing at all, so an existing precision could never be + // removed once set. + ...(wantsPrecision && (typeChanged || precisionChanged) + ? { precision: parsedPrecision ?? null } + : {}), + ...(wantsIncludeTime && (typeChanged || includeTimeChanged) + ? { includeTime: includeTimeInput } + : {}), } if (Object.keys(updates).length === 0) { onClose() @@ -306,6 +368,52 @@ function ColumnConfigBody({ )} + {wantsPrecision && ( + <> + +
+ + setPrecisionInput(e.target.value)} + placeholder='As stored' + inputClassName='[appearance:textfield] [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none' + /> +
+ + )} + + {wantsIncludeTime && ( + <> + +
+
+ + setIncludeTimeInput(!!v)} + /> +
+ {/* Turning this off rewrites every cell and cannot be undone by + turning it back on — the times are gone. Shown only when the + save would actually perform that rewrite. */} + {baselineIncludeTime && !includeTimeInput && ( +

+ Saving will remove the time from every cell in this column. This can’t be undone. +

+ )} +
+ + )} + {wantsOptions && ( <> diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx index 092e7046e79..006f27d4d0f 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx @@ -18,8 +18,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { useParams } from 'next/navigation' import type { ColumnDefinition, TableInfo, TableRow } from '@/lib/table' -import { columnTypeOf } from '@/lib/table/column-types' -import { resolveCurrencyCode } from '@/lib/table/currency' +import { columnTypeOf, describeColumnType } from '@/lib/table/column-types' import { useTimezone } from '@/hooks/queries/general-settings' import { useDeleteTableRow, useDeleteTableRows, useUpdateTableRow } from '@/hooks/queries/tables' import { @@ -211,13 +210,11 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) { )} ) - // Currency names its code — the modal edits the bare amount, so without it - // there is nothing on screen saying which currency the number is in. - const typeLabel = - column.type === 'currency' - ? `currency (${resolveCurrencyCode(column.currencyCode)})` - : column.type - const hint = `Type: ${typeLabel}${column.required ? '' : ' (optional)'}` + // The type's own description, which folds in the configuration that changes + // what a cell means — a currency's code, a date's time, a number's decimals. + // This also reads the registry's LABEL rather than the raw id, so the hint + // says "Text" and not "string". + const hint = `Type: ${describeColumnType(column)}${column.required ? '' : ' (optional)'}` const definition = columnTypeOf(column) if (definition.editor === 'toggle') { @@ -243,7 +240,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) { // The one type wanting a mono multi-line field; `editor: 'text'` covers both // this and a plain input, so it stays explicit rather than inventing a field // only one type would ever set. - if (column.type === 'json') { + if (definition.editor === 'json') { return (
@@ -272,17 +274,23 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) { flush className='flex-1' /> - - onChange( - localPartsToDateValue(parts.day ?? todayLocalCalendarDate(timeZone), time, timeZone) - ) - } - placeholder='Add time' - flush - className='w-[110px]' - /> + {withTime && ( + + onChange( + localPartsToDateValue( + parts.day ?? todayLocalCalendarDate(timeZone), + time, + timeZone + ) + ) + } + placeholder='Add time' + flush + className='w-[110px]' + /> + )}
) @@ -308,7 +316,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) { inputType={ definition.inputMode === 'decimal' && !definition.acceptsFormattedInput ? 'number' : 'text' } - value={formatValueForInput(value, column.type)} + value={formatValueForInput(value, column)} onChange={onChange} placeholder={`Enter ${column.name}`} /> diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/select-field/select-color-picker.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/select-field/select-color-picker.tsx new file mode 100644 index 00000000000..c091fdb1a41 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/select-field/select-color-picker.tsx @@ -0,0 +1,68 @@ +'use client' + +import { + Badge, + DropdownMenu, + DropdownMenuContent, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, + DropdownMenuTrigger, +} from '@sim/emcn' +// Deep import, not the `@/lib/table` barrel — see select-options-editor.tsx. +import { SELECT_OPTION_COLORS, type SelectOptionColor } from '@/lib/table/types' + +interface SelectColorPickerProps { + color: SelectOptionColor | undefined + onChange: (color: SelectOptionColor) => void + /** Option name, so the trigger's accessible label says which option it colors. */ + optionName: string +} + +/** Sentence-cased for display; the stored value stays the lowercase token. */ +function labelFor(color: SelectOptionColor): string { + return color.charAt(0).toUpperCase() + color.slice(1) +} + +/** + * Colour picker for one option's pill. + * + * A named list rather than a bare swatch grid, for two reasons. Colour is then + * not the only signal — the name is readable when the swatches are not + * distinguishable to the viewer — and it lets the menu use the real + * `DropdownMenuRadioItem` primitive: mutually exclusive `menuitemradio` + * semantics, roving focus and typeahead, a visible selected indicator, and + * close-on-select. Raw `