Skip to content

Commit c64d42c

Browse files
committed
fix(tables): check conversions against the target column, not a rebuilt stub
Bugbot flagged that the conversion gate calls `isValueCompatibleWithType`, which hand-builds a column from loose arguments and never sets `currencyCode` — while the coercion a few lines later reads the real one. The reported impact does not occur. Whether an amount parses does not depend on the currency, only which number it yields, so the gate and the coercion always agree on accept/reject; and the value written back is already coerced against the real column, so no cell is stored wrong. I checked this rather than argued it: across nine currency codes and every separator shape in the parser's repertoire, acceptance never diverged. The stub is still worth removing. It is correct only because of an invariant nobody wrote down, and `convertedColumn` — sitting right there, whose own comment already says it exists so the scan reads the same option set and currency — is what the gate should have used all along. So the gate now takes the target column itself. The loose-argument form moves into the test file as local marshalling, leaving production with only the whole-column shape, which cannot silently drop a key a future type adds. No behavior change, so no test changes: a currency-divergence regression test could not fail today, and pinning an invariant this removes the need for would only block a legitimate future change.
1 parent 6a47267 commit c64d42c

2 files changed

Lines changed: 38 additions & 30 deletions

File tree

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

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,32 @@ import { describe, expect, it } from 'vitest'
88
import { COLUMN_TYPE_REGISTRY } from '@/lib/table/column-types'
99
import {
1010
applyPendingRename,
11-
isValueCompatibleWithType,
11+
isValueCompatibleWithColumn,
1212
selectValueForConversion,
1313
} from '@/lib/table/columns/service'
1414
import { resolveSelectOptionId } from '@/lib/table/select-options'
15-
import type { ColumnDefinition, SelectOption } from '@/lib/table/types'
15+
import type { ColumnDefinition, ColumnType, SelectOption } from '@/lib/table/types'
16+
17+
/**
18+
* Marshals the loose per-type arguments these cases are written in into the
19+
* target column the gate takes. Lives here rather than in the service so
20+
* production has only the whole-column form, which cannot drop a metadata key.
21+
*/
22+
function isValueCompatibleWithType(
23+
value: unknown,
24+
targetType: ColumnType,
25+
targetOptions: SelectOption[] = [],
26+
targetMultiple = false,
27+
targetRequired = false
28+
): boolean {
29+
return isValueCompatibleWithColumn(value, {
30+
name: '',
31+
type: targetType,
32+
options: targetOptions,
33+
multiple: targetMultiple,
34+
required: targetRequired,
35+
})
36+
}
1637

1738
const OPTIONS: SelectOption[] = [
1839
{ id: 'opt_a', name: 'Alpha' },

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

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -801,15 +801,7 @@ export async function updateColumnType(
801801

802802
const effective = convertingAwayFromSelect ? selectValueForConversion(column, value) : value
803803

804-
if (
805-
!isValueCompatibleWithType(
806-
effective,
807-
data.newType,
808-
targetOptions,
809-
!!targetMultiple,
810-
targetRequired
811-
)
812-
) {
804+
if (!isValueCompatibleWithColumn(effective, convertedColumn)) {
813805
// A cell the target cannot read but that is merely EMPTY is not a
814806
// conversion failure — the write path already turns an unreadable value
815807
// into null on an optional column, so the conversion does the same. Only
@@ -1330,29 +1322,24 @@ export function selectValueForConversion(column: ColumnDefinition, value: unknow
13301322
}
13311323

13321324
/**
1333-
* Checks if a value is compatible with a target column type. For `select`,
1334-
* `targetOptions` is the option set the column will carry after the change: a
1335-
* value is compatible only if every part of it resolves to one of those options
1336-
* (by id or name). This blocks a conversion that would otherwise strand or
1337-
* silently drop values on the next row write.
1325+
* Checks a value against the column definition the table will end up with.
1326+
*
1327+
* Takes the whole target column rather than loose per-type arguments: the gate
1328+
* has to read the SAME metadata the later coercion does, and a hand-built stub
1329+
* silently omits whatever key its author did not think of. Today that key would
1330+
* be `currencyCode` — a three-decimal column reads `0,500` as a half dinar
1331+
* while a stub without the code reads it as five hundred.
1332+
*
1333+
* That divergence is currently invisible, because whether an amount parses at
1334+
* all does not depend on the currency, only which number it yields — and the
1335+
* write-back already coerces against the real column. Passing the real column
1336+
* here means it cannot become visible when that stops being true.
13381337
*
13391338
* Callers converting *away* from `select` must pass the resolved option
13401339
* name(s), not the stored ids — see {@link selectValueForConversion}.
13411340
*/
1342-
export function isValueCompatibleWithType(
1343-
value: unknown,
1344-
targetType: (typeof COLUMN_TYPES)[number],
1345-
targetOptions: SelectOption[] = [],
1346-
targetMultiple = false,
1347-
targetRequired = false
1348-
): boolean {
1341+
export function isValueCompatibleWithColumn(value: unknown, target: ColumnDefinition): boolean {
13491342
if (value === null || value === undefined) return true
13501343
// Each type reads only the metadata it owns.
1351-
return isValueCompatible(value, {
1352-
name: '',
1353-
type: targetType,
1354-
options: targetOptions,
1355-
multiple: targetMultiple,
1356-
required: targetRequired,
1357-
})
1344+
return isValueCompatible(value, target)
13581345
}

0 commit comments

Comments
 (0)