Skip to content

Commit f69197e

Browse files
committed
fix(tables): stop a blank cell blocking an optional type conversion
Found by Cursor Bugbot. `''` is incompatible with every numeric type, and the compatibility scan counted it as a hard blocker regardless of whether the target was optional — so a text column with a single empty cell could not be converted to a number at all, and the error said 'to a required ...' either way. An unreadable-but-empty cell is not a conversion failure. The write path already turns an unreadable value into null on an optional column, so the conversion now does the same and records null for it. A required target still reports it, which the existing guard above already does with the message that actually fits. Also pins the two intentional divergences from the pre-registry behavior. A differential run of the registry against the pre-refactor implementations (55 values x 7 column shapes) found ZERO coercion differences and exactly two compatibility differences, both deliberate: boolean now rejects the '1'/'0' conversions the old gate accepted and then nulled, and date now accepts the epoch numbers its write path always accepted. Tests pin both so neither can be silently reverted or widened.
1 parent 6c5fc2a commit f69197e

3 files changed

Lines changed: 67 additions & 2 deletions

File tree

apps/sim/lib/table/__tests__/column-conversion.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,25 @@ describe('isValueCompatibleWithType — currency', () => {
139139
expect(isValueCompatibleWithType(1234.56, 'number')).toBe(true)
140140
})
141141
})
142+
143+
describe('blank cells during conversion', () => {
144+
// A text column with a single empty cell could not be converted to a number
145+
// at all: `''` is incompatible with every numeric type, and the scan counted
146+
// it as a hard blocker even when the target was optional — reporting it with
147+
// an error that said "to a required ..." regardless.
148+
it('treats an empty cell as incompatible with the numeric types', () => {
149+
for (const type of ['number', 'currency'] as const) {
150+
expect(isValueCompatibleWithType('', type)).toBe(false)
151+
}
152+
})
153+
154+
it('still accepts an empty string for text, which can legitimately hold it', () => {
155+
expect(isValueCompatibleWithType('', 'string')).toBe(true)
156+
expect(isValueCompatibleWithType('', 'json')).toBe(true)
157+
})
158+
159+
it('lets a cleared select cell through only when the target is optional', () => {
160+
expect(isValueCompatibleWithType('', 'select', OPTIONS, false, false)).toBe(true)
161+
expect(isValueCompatibleWithType('', 'select', OPTIONS, false, true)).toBe(false)
162+
})
163+
})

apps/sim/lib/table/__tests__/column-type-registry.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
COLUMN_TYPES,
1818
columnTypeById,
1919
isColumnType,
20+
isValueCompatible,
2021
} from '@/lib/table/column-types'
2122
import type { ColumnDefinition } from '@/lib/table/types'
2223
import { validateColumnDefinition } from '@/lib/table/validation'
@@ -106,6 +107,38 @@ describe('conversion write-back', () => {
106107
})
107108
})
108109

110+
describe('intentional divergences from the pre-registry behavior', () => {
111+
// A differential run of the registry against the pre-refactor implementations
112+
// (55 values x 7 column shapes) found ZERO coercion differences and exactly
113+
// these compatibility differences. Both are deliberate fixes; pinning them
114+
// here so neither can be silently reverted or quietly widened.
115+
116+
it('rejects boolean conversions the old gate accepted and then nulled', () => {
117+
// The old gate accepted '1'/'0'/1/0 but the write path only ever accepted
118+
// 'true'/'false' — so the conversion reported zero incompatible rows and
119+
// then nulled every one of them. Rejecting is the honest answer.
120+
const column: ColumnDefinition = { name: 'b', type: 'boolean' }
121+
for (const value of ['0', '1', 0, 1]) {
122+
expect(isValueCompatible(value, column)).toBe(false)
123+
expect(COLUMN_TYPE_REGISTRY.boolean.coerce(value as never, column).ok).toBe(false)
124+
}
125+
for (const value of [true, false, 'true', 'false']) {
126+
expect(isValueCompatible(value, column)).toBe(true)
127+
}
128+
})
129+
130+
it('accepts epoch numbers converting to date, which the old gate rejected', () => {
131+
// The write path already accepted epoch numbers; only the gate disagreed.
132+
// Now that they agree, the conversion is allowed — and safe, because the
133+
// conversion writes the coerced ISO string back rather than leaving a bare
134+
// number that `::timestamptz` cannot cast.
135+
const column: ColumnDefinition = { name: 'd', type: 'date' }
136+
expect(isValueCompatible(0, column)).toBe(true)
137+
const coerced = COLUMN_TYPE_REGISTRY.date.coerce(0 as never, column)
138+
expect(coerced.ok && typeof coerced.value).toBe('string')
139+
})
140+
})
141+
109142
describe('metadata ownership', () => {
110143
const column = (over: Partial<ColumnDefinition>): ColumnDefinition =>
111144
({ name: 'c', type: 'string', ...over }) as ColumnDefinition

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -646,8 +646,18 @@ export async function updateColumnType(
646646
targetRequired
647647
)
648648
) {
649-
if (effective === null || effective === '') blankCount++
650-
else incompatibleCount++
649+
// A cell the target cannot read but that is merely EMPTY is not a
650+
// conversion failure — the write path already turns an unreadable value
651+
// into null on an optional column, so the conversion does the same. Only
652+
// a required target has a real problem with it, and the guard above has
653+
// already reported those. Blocking here meant a text column with a
654+
// single blank cell could not be converted to a number at all.
655+
if (effective === null || effective === '') {
656+
if (targetRequired) blankCount++
657+
else coercedByRowId.set(row.id, null)
658+
} else {
659+
incompatibleCount++
660+
}
651661
continue
652662
}
653663

0 commit comments

Comments
 (0)