Skip to content

Commit 260ab82

Browse files
committed
fix(tables): stop inventing amounts from identifiers; fix the copilot retype
Adversarial pass over the final state, seven real findings. Two destroyed data. The copilot `update_column` still used the two-transaction pattern the HTTP routes were fixed for: `unique` was never forwarded to the typed write, so a retype+unique committed the conversion and then failed the constraint — the same irrecoverable half. It now rides the typed write, and the separate constraint write only runs when no typed write did. And the parser's three-letter strip removed ANY three letters, not an ISO code: `SKU400` parsed as 400, `ABC1234` as 1234. Converting a column of part numbers to currency rewrote every cell with an invented value — while the comment two lines above claimed a SKU was exactly what it prevented. The rule is now that a letter touching a digit means identifier, not amount; a currency marker is always separated by a space or a symbol. That same change fixed a class the review surfaced: the pinned currencies could not parse their own conventional notation. `R$ 1.234,56`, `1 234,56 kr`, `1234,56 zł`, `CHF 1’234.56` and Indian lakh grouping (`₹12,34,567.89`) all work now — these are what Intl emits, so a paste from a spreadsheet was being rejected. `updateColumnConstraints` was a fourth copy of the constraint rules the shared helper exists to unify, and had already drifted: it hardcoded `type === 'select'` where the helper asks the registry, so a future type declaring `supportsUnique: false` would have been ignored on that path. It now uses the helper. `updateColumnType`'s unchanged-type early return silently discarded every field except the rename. Callers gate on the type changing, but from a read taken before the lock — so a concurrent change could land there with real work pending and answer success. It now throws. Also: `UpdateColumnCurrencyData` was missing `required`, which only compiled because the routes pass it through a spread; a missing column returns 404 instead of a 400 reading "of type undefined"; and the comments describing the old two-transaction architecture are gone. Verified NOT a bug: CSV export of a currency column writes the raw number, so export/import round-trips losslessly.
1 parent 6df5bd9 commit 260ab82

7 files changed

Lines changed: 122 additions & 57 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
136136
// The constraints write below is a separate, unconditional step, so it is
137137
// the last one whenever it runs — that is the write the rename rides on.
138138
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
139+
if (!currentColumn) {
140+
return NextResponse.json(
141+
{ error: `Column "${validated.columnName}" not found` },
142+
{ status: 404 }
143+
)
144+
}
145+
139146
// A retype applies and validates the constraints itself, so the separate
140147
// constraint write only runs when the type is unchanged. The rename rides
141148
// whichever write actually runs last.

apps/sim/app/api/v1/tables/[tableId]/columns/route.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
170170
// The constraints write below is a separate, unconditional step, so it is
171171
// the last one whenever it runs — that is the write the rename rides on.
172172
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
173+
if (!currentColumn) {
174+
return NextResponse.json(
175+
{ error: `Column "${validated.columnName}" not found` },
176+
{ status: 404 }
177+
)
178+
}
179+
173180
// A retype applies and validates the constraints itself, so the separate
174181
// constraint write only runs when the type is unchanged. The rename rides
175182
// whichever write actually runs last.

apps/sim/lib/copilot/tools/server/table/user-table.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1728,6 +1728,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
17281728
options,
17291729
multiple,
17301730
...(currencyCode !== undefined ? { currencyCode } : {}),
1731+
...(uniqFlag !== undefined ? { unique: uniqFlag } : {}),
17311732
},
17321733
requestId
17331734
)
@@ -1742,7 +1743,12 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
17421743
}
17431744
assertNotAborted()
17441745
result = await updateColumnCurrency(
1745-
{ tableId: args.tableId, columnName: colName, currencyCode },
1746+
{
1747+
tableId: args.tableId,
1748+
columnName: colName,
1749+
currencyCode,
1750+
...(uniqFlag !== undefined ? { unique: uniqFlag } : {}),
1751+
},
17461752
requestId
17471753
)
17481754
} else if (options !== undefined || multiple !== undefined) {
@@ -1759,11 +1765,20 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
17591765
}
17601766
assertNotAborted()
17611767
result = await updateColumnOptions(
1762-
{ tableId: args.tableId, columnName: colName, options: nextOptions, multiple },
1768+
{
1769+
tableId: args.tableId,
1770+
columnName: colName,
1771+
options: nextOptions,
1772+
multiple,
1773+
...(uniqFlag !== undefined ? { unique: uniqFlag } : {}),
1774+
},
17631775
requestId
17641776
)
17651777
}
1766-
if (uniqFlag !== undefined) {
1778+
// Skipped when a typed write ran: that write already applied and
1779+
// validated the constraint, in one transaction with the change it
1780+
// accompanies. Mirrors the HTTP columns routes.
1781+
if (uniqFlag !== undefined && result === undefined) {
17671782
assertNotAborted()
17681783
result = await updateColumnConstraints(
17691784
{ tableId: args.tableId, columnName: colName, unique: uniqFlag },

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

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,38 @@ describe('parseCurrencyInput', () => {
103103
expect(parseCurrencyInput('-$12.50')).toBe(-12.5)
104104
})
105105

106+
it('parses the locale formats of the currencies the picker pins', () => {
107+
// These are exactly what `Intl.NumberFormat` emits, i.e. what a user pastes
108+
// from a spreadsheet. Rejecting them would make the pinned currencies
109+
// unusable in their own conventional notation.
110+
expect(parseCurrencyInput('R$ 1.234,56')).toBe(1234.56)
111+
expect(parseCurrencyInput('₹12,34,567.89')).toBe(1234567.89)
112+
expect(parseCurrencyInput('1 234,56 kr')).toBe(1234.56)
113+
expect(parseCurrencyInput('1.234,56 kr.')).toBe(1234.56)
114+
expect(parseCurrencyInput('1234,56 zł')).toBe(1234.56)
115+
expect(parseCurrencyInput('CHF 1’234.56')).toBe(1234.56)
116+
})
117+
118+
it('rejects an identifier whose letters touch its digits', () => {
119+
// The distinguishing rule: a currency marker is always separated from the
120+
// number by a space or a symbol, so letters touching digits mean this is a
121+
// part number, not an amount. Without it, converting a column of SKUs to
122+
// currency rewrote every cell with an invented value.
123+
expect(parseCurrencyInput('SKU400')).toBeNull()
124+
expect(parseCurrencyInput('ABC1234')).toBeNull()
125+
expect(parseCurrencyInput('A1B2')).toBeNull()
126+
// A marker separated properly still parses.
127+
expect(parseCurrencyInput('USD 400')).toBe(400)
128+
expect(parseCurrencyInput('$400')).toBe(400)
129+
})
130+
106131
it('rejects text that merely contains digits', () => {
107132
// Scraping digits out of arbitrary text invents a value. A string column of
108133
// SKUs, phone numbers, or US-format dates converting to currency would
109134
// otherwise report zero incompatible rows and rewrite every cell.
110135
expect(parseCurrencyInput('01/02/2024')).toBeNull()
111136
expect(parseCurrencyInput('Room 101')).toBeNull()
112137
expect(parseCurrencyInput('Invoice 2024')).toBeNull()
113-
expect(parseCurrencyInput('A1B2')).toBeNull()
114138
expect(parseCurrencyInput('1_000')).toBeNull()
115139
})
116140

@@ -121,9 +145,10 @@ describe('parseCurrencyInput', () => {
121145
expect(parseCurrencyInput('1,,2')).toBeNull()
122146
expect(parseCurrencyInput('0.1.2')).toBeNull()
123147
expect(parseCurrencyInput('1,000,00')).toBeNull()
124-
// Valid grouping still works.
148+
// Valid grouping still works, western and Indian.
125149
expect(parseCurrencyInput('1.234.567')).toBe(1234567)
126150
expect(parseCurrencyInput('1,234,567.89')).toBe(1234567.89)
151+
expect(parseCurrencyInput('12,34,567')).toBe(1234567)
127152
})
128153

129154
it('rejects values carrying no amount', () => {

apps/sim/lib/table/columns/service.ts

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -701,8 +701,23 @@ export async function updateColumnType(
701701

702702
const column = schema.columns[columnIndex]
703703
if (column.type === data.newType) {
704-
// The type is unchanged, but a rename folded into this same request still
705-
// has to land — returning here unconditionally would drop it silently.
704+
// Callers gate on the type actually changing, but they compute that from
705+
// a schema read taken before this transaction took the lock — so a
706+
// concurrent change can land us here with real work still to do. Only a
707+
// rename can be honoured without a conversion; anything else would be
708+
// silently discarded, and answering success for a change that never
709+
// happened is the worst outcome available.
710+
const carriesOtherWork =
711+
data.required !== undefined ||
712+
data.unique !== undefined ||
713+
data.options !== undefined ||
714+
data.multiple !== undefined ||
715+
data.currencyCode !== undefined
716+
if (carriesOtherWork) {
717+
throw new Error(
718+
`Column "${column.name}" is already type "${data.newType}"; re-issue the request without a type change.`
719+
)
720+
}
706721
const renamed = applyPendingRename(schema.columns, columnIndex, data.newName)
707722
if (renamed === column) return table
708723
return persistColumns(
@@ -925,41 +940,8 @@ export async function updateColumnConstraints(
925940

926941
const column = schema.columns[columnIndex]
927942
const columnKey = getColumnId(column)
928-
if (column.workflowGroupId) {
929-
throw new Error(
930-
`Cannot change constraints on workflow-output column "${column.name}". Constraints aren't applicable to columns whose values come from workflow execution.`
931-
)
932-
}
933-
if (data.required === true && !column.required) {
934-
const emptyCount = await countEmptyCells(trx, data.tableId, columnKey)
935-
if (emptyCount > 0) {
936-
throw new Error(
937-
`Cannot set column "${column.name}" as required: ${emptyCount} row(s) have null, missing, or empty values`
938-
)
939-
}
940-
}
941-
942-
if (data.unique === true && column.type === 'select') {
943-
throw new Error(
944-
`Cannot set column "${column.name}" as unique: select columns compare stored option ids, which would allow only one row per option.`
945-
)
946-
}
947-
948-
if (data.unique === true && !column.unique) {
949-
if (await hasDuplicateValues(trx, data.tableId, columnKey)) {
950-
throw new Error(`Cannot set column "${column.name}" as unique: duplicate values exist`)
951-
}
952-
}
953-
954-
const withConstraints = schema.columns.map((c, i) =>
955-
i === columnIndex
956-
? {
957-
...c,
958-
...(data.required !== undefined ? { required: data.required } : {}),
959-
...(data.unique !== undefined ? { unique: data.unique } : {}),
960-
}
961-
: c
962-
)
943+
const constrained = await applyConstraints(trx, data.tableId, column, columnKey, data)
944+
const withConstraints = schema.columns.map((c, i) => (i === columnIndex ? constrained : c))
963945
const updatedColumns = withConstraints.map((c, i) =>
964946
i === columnIndex ? applyPendingRename(withConstraints, columnIndex, data.newName) : c
965947
)

apps/sim/lib/table/currency.ts

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,20 @@ export const DEFAULT_CURRENCY_CODE = 'USD'
1515
/** A bare numeric literal in exponent form, e.g. `1e+21` or `-1.5e-3`. */
1616
const EXPONENT_LITERAL = /^[+-]?\d+(?:\.\d+)?[eE][+-]?\d+$/
1717

18+
/** A letter directly adjacent to a digit: an identifier, not an amount. */
19+
const LETTER_TOUCHING_DIGIT = /\p{L}\d|\d\p{L}/u
20+
21+
/**
22+
* A leading currency marker: up to three letters and/or a symbol, optionally
23+
* behind a sign. The sign is captured so `-$12.50` keeps it.
24+
*/
25+
const CURRENCY_MARKER_PREFIX =
26+
/^[\s\u00a0\u202f]*([+-]?)[\s\u00a0\u202f]*(?:\p{Sc}\p{L}{0,3}|\p{L}{1,3}\p{Sc}?)[\s\u00a0\u202f]*/u
27+
28+
/** The same, trailing. */
29+
const CURRENCY_MARKER_SUFFIX =
30+
/[\s\u00a0\u202f]*(?:\p{Sc}\p{L}{0,3}|\p{L}{1,3}\p{Sc}?)\.?[\s\u00a0\u202f]*$/u
31+
1832
/** Only digits and separators, with at least one digit. */
1933
const AMOUNT_SHAPE = /^[+-]?[\d.,]*\d[\d.,]*$/
2034

@@ -26,7 +40,17 @@ const AMOUNT_SHAPE = /^[+-]?[\d.,]*\d[\d.,]*$/
2640
function hasValidGrouping(text: string, separator: string): boolean {
2741
const groups = text.split(separator)
2842
if (groups.length === 1) return true
29-
return /^\d{1,3}$/.test(groups[0]) && groups.slice(1).every((group) => /^\d{3}$/.test(group))
43+
if (!/^\d{1,3}$/.test(groups[0])) return false
44+
const rest = groups.slice(1)
45+
// Western: every later group is exactly three.
46+
if (rest.every((group) => /^\d{3}$/.test(group))) return true
47+
// Indian: the final group is three and the ones before it are two —
48+
// `12,34,567`. Still rejects `1,000,00`, whose final group is two.
49+
return (
50+
rest.length > 1 &&
51+
/^\d{3}$/.test(rest[rest.length - 1]) &&
52+
rest.slice(0, -1).every((group) => /^\d{2}$/.test(group))
53+
)
3054
}
3155

3256
/** A decimal/grouping separator followed by whitespace — a list, not an amount. */
@@ -173,13 +197,21 @@ export function parseCurrencyInput(raw: unknown): number | null {
173197
// is what distinguishes this from the `E` inside an ISO code like `12 EUR`.
174198
if (INTERIOR_EXPONENT.test(body)) return null
175199

176-
// What remains once currency symbols, spacing, and an ISO code are removed
177-
// must be ONLY digits and separators. Scraping digits out of anything else
178-
// invents a value: a US-format date, a SKU, or a room number would all parse.
200+
// A letter touching a digit means this is an identifier, not an amount —
201+
// `SKU400`, `ABC1234`. Currency markers are always separated from the number
202+
// by a space or a symbol, so this distinguishes them without a symbol list.
203+
if (LETTER_TOUCHING_DIGIT.test(body)) return null
204+
205+
// Strip the currency marker: up to three letters (an ISO code, `kr`, `zł`)
206+
// optionally joined to a symbol (`R$`, `CHF`), at either end. Bounded at
207+
// three so prose does not qualify — `Revenue 5` keeps its letters and is
208+
// rejected below.
179209
const cleaned = body
180-
.replace(/\p{Sc}/gu, '')
181-
.replace(/\s/g, '')
182-
.replace(/^[A-Za-z]{3}|[A-Za-z]{3}$/g, '')
210+
.replace(CURRENCY_MARKER_PREFIX, '$1')
211+
.replace(CURRENCY_MARKER_SUFFIX, '')
212+
.replace(/[\s\u00a0\u202f\u2019']/gu, '')
213+
// What remains must be ONLY digits and separators. Anything else — a
214+
// US-format date, leftover prose — is not an amount.
183215
if (!AMOUNT_SHAPE.test(cleaned)) return null
184216

185217
const signed = /^[+-]/.test(cleaned)

apps/sim/lib/table/types.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -800,10 +800,8 @@ export interface UpdateColumnTypeData {
800800
*/
801801
unique?: boolean
802802
/**
803-
* The `required` value the same request is about to set, when it changes type
804-
* and constraints together. Those are separate transactions, so the
805-
* conversion has to validate against the constraint the column will END UP
806-
* with — otherwise it commits and the constraint write then fails.
803+
* The `required` value the same request is about to set. Applied by this
804+
* write, in the same transaction as the change it accompanies.
807805
*/
808806
required?: boolean
809807
}
@@ -822,10 +820,8 @@ export interface UpdateColumnOptionsData {
822820
/** Toggle single/multi selection alongside the options update. */
823821
multiple?: boolean
824822
/**
825-
* The `required` value the same request is about to set. The constraint write
826-
* is a separate transaction, so the options update has to validate against
827-
* the constraint the column will END UP with — otherwise it clears cells and
828-
* the constraint write then fails, leaving the removal committed.
823+
* The `required` value the same request is about to set. Applied by this
824+
* write, in the same transaction as the options change.
829825
*/
830826
required?: boolean
831827
}
@@ -845,6 +841,7 @@ export interface UpdateColumnCurrencyData {
845841
newName?: string
846842
/** Constraints to apply in the SAME transaction as this write. */
847843
unique?: boolean
844+
required?: boolean
848845
currencyCode: string
849846
}
850847

0 commit comments

Comments
 (0)