Skip to content

Commit c9fec0f

Browse files
committed
fix(tables): address Bugbot round 1
Retype's stale-work guard hardcoded options/multiple/currencyCode, so a request carrying `precision` or `includeTime` against an already-correct type hit the rename-only path and was silently discarded while reporting success — the exact hand-listed-key bug this PR exists to remove. It reads TYPE_SPECIFIC_COLUMN_KEYS now. Filter values are read through the column that owns them. `parseScalar` returns NaN for `50%`, `1h` and `$1,234.56`, so the value stayed a string, met the numeric cast, and the range filter was rejected outright. Only cast columns take this path — a text column keeps parseScalar's existing coercion and a select keeps its option id verbatim. CSV import was the one write path that bypassed `applyIncludeTime`, so an import could put an instant into a column whose schema says it holds calendar days. Decimal places is now clearable: the update sent nothing when the field was emptied, so a precision could be set but never unset. `null` on the update contract removes a key, applied in `updateColumnMetadata` and mirrored in the optimistic cache. Non-finite input no longer clamps to 0 and saves as "zero decimal places". Filter pruning also moved off `type === 'select'` onto storesOpaqueIds / storesMultipleValues, which the earlier sweep had missed.
1 parent 9d90033 commit c9fec0f

13 files changed

Lines changed: 220 additions & 53 deletions

File tree

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,12 @@ import {
2020
updateColumnType,
2121
} from '@/lib/table'
2222
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
23-
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
23+
import {
24+
columnTypeById,
25+
metadataKeysIn,
26+
metadataWithoutClears,
27+
pickMetadata,
28+
} from '@/lib/table/column-types'
2429
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
2530
import { signalTableSchemaChanged } from '@/lib/table/events'
2631
import {
@@ -216,7 +221,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
216221
// Every type-specific key the payload carries, whichever writer would
217222
// own it standalone: a conversion applies its target's metadata in the
218223
// same transaction rather than leaving it to a second write.
219-
...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]),
224+
...metadataWithoutClears(
225+
pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys])
226+
),
220227
// Forwarded so the conversion validates against the constraint this
221228
// same request is about to set, not the column's current one.
222229
...(updates.required !== undefined ? { required: updates.required } : {}),

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,12 @@ import {
1919
updateColumnType,
2020
} from '@/lib/table'
2121
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
22-
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
22+
import {
23+
columnTypeById,
24+
metadataKeysIn,
25+
metadataWithoutClears,
26+
pickMetadata,
27+
} from '@/lib/table/column-types'
2328
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
2429
import { signalTableSchemaChanged } from '@/lib/table/events'
2530
import {
@@ -250,7 +255,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
250255
// Every type-specific key the payload carries, whichever writer would
251256
// own it standalone: a conversion applies its target's metadata in the
252257
// same transaction rather than leaving it to a second write.
253-
...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]),
258+
...metadataWithoutClears(
259+
pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys])
260+
),
254261
// Forwarded so the conversion validates against the constraint this
255262
// same request is about to set, not the column's current one.
256263
...(updates.required !== undefined ? { required: updates.required } : {}),

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/column-config-sidebar/column-config-sidebar.tsx

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,15 @@ function ColumnConfigBody({
159159
// type that loses it cannot leave a stale control behind.
160160
const wantsCurrency = typeOwnsMetadataKey(typeInput, 'currencyCode')
161161
const wantsPrecision = typeOwnsMetadataKey(typeInput, 'precision')
162+
// `undefined` means "no precision declared" — the field is legitimately
163+
// clearable back to rendering values as stored. A non-empty value that is not
164+
// a finite number is treated the same rather than clamping to 0, so garbage
165+
// never silently saves as "zero decimal places".
166+
const precisionNumber = Number(precisionInput)
162167
const parsedPrecision =
163-
precisionInput.trim() === '' ? undefined : clampPrecision(Number(precisionInput))
168+
precisionInput.trim() === '' || !Number.isFinite(precisionNumber)
169+
? undefined
170+
: clampPrecision(precisionNumber)
164171
const wantsIncludeTime = typeOwnsMetadataKey(typeInput, 'includeTime')
165172
const trimmedOptions = optionsInput.map((o) => ({ ...o, name: o.name.trim() }))
166173

@@ -231,7 +238,7 @@ function ColumnConfigBody({
231238
options?: SelectOption[]
232239
multiple?: boolean
233240
currencyCode?: string
234-
precision?: number
241+
precision?: number | null
235242
includeTime?: boolean
236243
} = {
237244
...(renamed ? { name: trimmedName } : {}),
@@ -243,8 +250,11 @@ function ColumnConfigBody({
243250
...(wantsCurrency && (typeChanged || currencyChanged)
244251
? { currencyCode: currencyInput }
245252
: {}),
246-
...(wantsPrecision && (typeChanged || precisionChanged) && parsedPrecision !== undefined
247-
? { precision: parsedPrecision }
253+
// `null` clears the key. Gating on `!== undefined` meant emptying the
254+
// field sent nothing at all, so an existing precision could never be
255+
// removed once set.
256+
...(wantsPrecision && (typeChanged || precisionChanged)
257+
? { precision: parsedPrecision ?? null }
248258
: {}),
249259
...(wantsIncludeTime && (typeChanged || includeTimeChanged)
250260
? { includeTime: includeTimeInput }

apps/sim/hooks/queries/tables.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ import {
8686
} from '@/lib/api/contracts/tables'
8787
import { buildUpgradeHref } from '@/lib/billing/upgrade-reasons'
8888
import type {
89+
ColumnDefinition,
8990
CsvHeaderMapping,
9091
EnrichmentRunDetail,
9192
RowData,
@@ -1365,7 +1366,15 @@ export function useUpdateColumn({ workspaceId, tableId }: RowMutationContext) {
13651366
const isRename = typeof (updates as { name?: string }).name === 'string'
13661367
const nextColumns = previousDetail.schema.columns.map((c) => {
13671368
if (getColumnId(c) !== columnName && c.name.toLowerCase() !== lower) return c
1368-
const next = { ...c, ...updates }
1369+
// A `null` in the payload CLEARS its key server-side, so the
1370+
// optimistic column must drop it too — spreading the null straight
1371+
// in would leave the grid rendering against a shape the server will
1372+
// never return.
1373+
const next: ColumnDefinition = { ...c }
1374+
for (const [key, value] of Object.entries(updates)) {
1375+
if (value === null) delete next[key as keyof ColumnDefinition]
1376+
else Object.assign(next, { [key]: value })
1377+
}
13691378
if (isRename && next.id === undefined) next.id = getColumnId(c)
13701379
return next
13711380
})

apps/sim/lib/api/contracts/tables.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import {
3838
TABLE_LIMITS,
3939
} from '@/lib/table/constants'
4040
import { CSV_MAX_FILE_SIZE_BYTES } from '@/lib/table/import'
41-
import type { ColumnTypeMetadata } from '@/lib/table/types'
41+
import type { ColumnMetadataPatch } from '@/lib/table/types'
4242
import { SELECT_OPTION_COLORS } from '@/lib/table/types'
4343

4444
export const domainObjectSchema = <T>() => z.custom<T>(isRecordLike)
@@ -103,7 +103,7 @@ function refineColumnOptions(
103103
type?: (typeof COLUMN_TYPES)[number]
104104
options?: z.infer<typeof selectOptionsSchema>
105105
multiple?: boolean
106-
} & ColumnTypeMetadata,
106+
} & ColumnMetadataPatch,
107107
ctx: z.RefinementCtx
108108
): void {
109109
// Keys the type cannot be created without, checked before the ownership sweep
@@ -321,7 +321,10 @@ export const updateTableColumnBodySchema = z.object({
321321
unique: z.boolean().optional(),
322322
options: selectOptionsSchema.optional(),
323323
multiple: z.boolean().optional(),
324-
precision: precisionSchema.optional(),
324+
/** `null` clears the setting, returning the column to rendering values as
325+
* stored. Only the UPDATE surface accepts it — on a create there is
326+
* nothing to clear. */
327+
precision: precisionSchema.nullable().optional(),
325328
includeTime: z.boolean().optional(),
326329
currencyCode: currencyCodeSchema.optional(),
327330
})

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import { columnTypeForLeaf, deriveOutputColumnName } from '@/lib/table/column-na
3737
import {
3838
columnTypeById,
3939
metadataKeysIn,
40+
metadataWithoutClears,
4041
pickMetadata,
4142
TYPE_SPECIFIC_COLUMN_KEYS,
4243
validateTypeMetadata,
@@ -1787,7 +1788,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
17871788
newType: newType as (typeof COLUMN_TYPES)[number],
17881789
options,
17891790
multiple,
1790-
...pickMetadata(metadataUpdates, genericMetadataKeys),
1791+
...metadataWithoutClears(pickMetadata(metadataUpdates, genericMetadataKeys)),
17911792
...(uniqFlag !== undefined ? { unique: uniqFlag } : {}),
17921793
},
17931794
requestId

apps/sim/lib/table/__tests__/column-types-contact.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import {
1616
isValueCompatible,
1717
ownersOfMetadataKey,
1818
} from '@/lib/table/column-types'
19+
import { coerceValue } from '@/lib/table/import'
20+
import { filterRulesToFilter } from '@/lib/table/query-builder/converters'
1921
import type { ColumnDefinition } from '@/lib/table/types'
2022

2123
const column = (type: ColumnDefinition['type'], extra: Partial<ColumnDefinition> = {}) =>
@@ -223,6 +225,52 @@ describe('audit regressions', () => {
223225
})
224226
})
225227

228+
describe('review-round regressions', () => {
229+
it('reads a formatted filter value through the column that owns it', () => {
230+
// `parseScalar` returns NaN for `50%` / `1h` / `$1,234.56`, so the value
231+
// stayed a string, hit the numeric cast, and the range filter was rejected.
232+
const cases: Array<[ColumnDefinition['type'], string, number]> = [
233+
['percent', '50%', 50],
234+
['duration', '1h', 3600],
235+
['duration', '1:30', 90],
236+
['currency', '$1,234.56', 1234.56],
237+
]
238+
for (const [type, typed, expected] of cases) {
239+
const col = column(type)
240+
const rules = [
241+
{ id: 'r1', logicalOperator: 'and' as const, column: 'c', operator: 'gte', value: typed },
242+
]
243+
const filter = filterRulesToFilter(rules, [{ ...col, id: 'c' }])
244+
expect(filter, `${type} ${typed}`).toEqual({ c: { $gte: expected } })
245+
}
246+
})
247+
248+
it('leaves text- and opaque-id columns on their existing coercion', () => {
249+
// The type-aware parse must apply ONLY to cast columns. A text column keeps
250+
// `parseScalar`'s long-standing number coercion, and a select keeps its
251+
// option id verbatim — an id of "1" coerced to a number would compare
252+
// against the stored JSON string by containment and match nothing.
253+
const eq = (value: string, col: ColumnDefinition) =>
254+
filterRulesToFilter(
255+
[{ id: 'r1', logicalOperator: 'and' as const, column: 'c', operator: 'eq', value }],
256+
[{ ...col, id: 'c' }]
257+
)
258+
expect(eq('123', column('string'))).toEqual({ c: 123 })
259+
expect(eq('1', column('select', { options: [{ id: '1', name: 'One' }] }))).toEqual({ c: '1' })
260+
})
261+
262+
it('truncates an imported date for a date-only column', () => {
263+
// CSV import was the one write path that bypassed `applyIncludeTime`.
264+
const dateOnly = column('date', { includeTime: false })
265+
expect(coerceValue('2024-01-15T13:45:00Z', 'date', { column: dateOnly })).toBe('2024-01-15')
266+
})
267+
268+
it('leaves an imported date alone for a column that carries time', () => {
269+
const withTime = column('date', { includeTime: true })
270+
expect(coerceValue('2024-01-15T13:45:00Z', 'date', { column: withTime })).toContain('13:45')
271+
})
272+
})
273+
226274
describe('date includeTime', () => {
227275
it('truncates to a calendar day only when includeTime is explicitly false', () => {
228276
const dateOnly = column('date', { includeTime: false })

apps/sim/lib/table/column-types/registry.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,12 @@ import type {
4141
} from '@/lib/table/column-types/types'
4242
import { COLUMN_TYPES, TYPE_SPECIFIC_COLUMN_KEYS } from '@/lib/table/column-types/types'
4343
import { urlColumnType } from '@/lib/table/column-types/url'
44-
import type { ColumnDefinition, JsonValue } from '@/lib/table/types'
44+
import type {
45+
ColumnDefinition,
46+
ColumnMetadataPatch,
47+
ColumnTypeMetadata,
48+
JsonValue,
49+
} from '@/lib/table/types'
4550

4651
export { COLUMN_TYPES }
4752
export { MULTI_SELECT_OPERATORS, SINGLE_SELECT_OPERATORS }
@@ -185,7 +190,7 @@ export function ownersOfMetadataKey(key: TypeSpecificColumnKey): ColumnTypeDefin
185190
* owner keeps its own writer — today only `select`'s `options` / `multiple`.
186191
* Callers branch on which set is non-empty instead of testing key names.
187192
*/
188-
export function metadataKeysIn(updates: Partial<ColumnDefinition>): {
193+
export function metadataKeysIn(updates: ColumnMetadataPatch): {
189194
generic: TypeSpecificColumnKey[]
190195
dedicated: TypeSpecificColumnKey[]
191196
} {
@@ -202,6 +207,22 @@ export function metadataKeysIn(updates: Partial<ColumnDefinition>): {
202207
return { generic, dedicated }
203208
}
204209

210+
/**
211+
* A metadata patch with its clears dropped, for the conversion path.
212+
*
213+
* On a retype, "remove this key" and "never had this key" are the same thing —
214+
* the converted column is rebuilt from the target type's owned keys — so a
215+
* `null` is simply not carried across.
216+
*/
217+
export function metadataWithoutClears(patch: ColumnMetadataPatch): ColumnTypeMetadata {
218+
const resolved: ColumnTypeMetadata = {}
219+
for (const key of TYPE_SPECIFIC_COLUMN_KEYS) {
220+
const value = patch[key]
221+
if (value !== undefined && value !== null) Object.assign(resolved, { [key]: value })
222+
}
223+
return resolved
224+
}
225+
205226
/** Whether a column of `type` may carry `key`. */
206227
export function typeOwnsMetadataKey(type: string | undefined, key: TypeSpecificColumnKey): boolean {
207228
return columnTypeById(type).ownedMetadata.includes(key)
@@ -215,10 +236,10 @@ export function typeOwnsMetadataKey(type: string | undefined, key: TypeSpecificC
215236
* this key" to `filterUndefined` further down the write path.
216237
*/
217238
export function pickMetadata(
218-
updates: Partial<ColumnDefinition>,
239+
updates: ColumnMetadataPatch,
219240
keys: readonly TypeSpecificColumnKey[]
220-
): Partial<ColumnDefinition> {
221-
const picked: Partial<ColumnDefinition> = {}
241+
): ColumnMetadataPatch {
242+
const picked: ColumnMetadataPatch = {}
222243
for (const key of keys) {
223244
if (updates[key] !== undefined) Object.assign(picked, { [key]: updates[key] })
224245
}

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,11 @@
1212
import {
1313
columnTypeById,
1414
metadataKeysIn,
15+
metadataWithoutClears,
1516
ownersOfMetadataKey,
1617
pickMetadata,
1718
} from '@/lib/table/column-types'
18-
import type { ColumnDefinition } from '@/lib/table/types'
19+
import type { ColumnDefinition, ColumnMetadataPatch } from '@/lib/table/types'
1920

2021
/**
2122
* Rejects a metadata update that the column's resulting type cannot accept.
@@ -35,7 +36,7 @@ import type { ColumnDefinition } from '@/lib/table/types'
3536
export function validateMetadataUpdate(
3637
currentColumn: ColumnDefinition,
3738
resultingType: string | undefined,
38-
updates: Partial<ColumnDefinition>
39+
updates: ColumnMetadataPatch
3940
): string | null {
4041
const { generic, dedicated } = metadataKeysIn(updates)
4142
const definition = columnTypeById(resultingType)
@@ -60,10 +61,15 @@ export function validateMetadataUpdate(
6061
// `updates` wholesale would fold in a pending `name`, so rejecting a bad
6162
// value would name the column by a rename that this very request is refusing
6263
// to perform.
64+
// A cleared key (`null`) validates as ABSENT — that is what the write will
65+
// leave behind — rather than as a null the type's validator has no arm for.
6366
const resulting: ColumnDefinition = {
6467
...currentColumn,
6568
...(resultingType ? { type: definition.id } : {}),
66-
...pickMetadata(updates, [...generic, ...dedicated]),
69+
...metadataWithoutClears(pickMetadata(updates, [...generic, ...dedicated])),
70+
}
71+
for (const key of [...generic, ...dedicated]) {
72+
if (updates[key] === null) delete resulting[key]
6773
}
6874
const errors = definition.validateDefinition?.(resulting) ?? []
6975
return errors[0] ?? null

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

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
columnTypeById,
2020
columnTypeOf,
2121
isValueCompatible,
22+
metadataWithoutClears,
2223
pickMetadata,
2324
TYPE_SPECIFIC_COLUMN_KEYS,
2425
type TypeSpecificColumnKey,
@@ -707,12 +708,14 @@ export async function updateColumnType(
707708
// rename can be honoured without a conversion; anything else would be
708709
// silently discarded, and answering success for a change that never
709710
// happened is the worst outcome available.
711+
// Read the metadata keys from the registry, not a hand-written list. The
712+
// three that used to be named here meant a newly declared key (a
713+
// `precision`, an `includeTime`) rode this path and was silently
714+
// discarded while the request reported success.
710715
const carriesOtherWork =
711716
data.required !== undefined ||
712717
data.unique !== undefined ||
713-
data.options !== undefined ||
714-
data.multiple !== undefined ||
715-
data.currencyCode !== undefined
718+
TYPE_SPECIFIC_COLUMN_KEYS.some((key) => data[key] !== undefined)
716719
if (carriesOtherWork) {
717720
throw new Error(
718721
`Column "${column.name}" is already type "${data.newType}"; re-issue the request without a type change.`
@@ -1164,12 +1167,24 @@ export async function updateColumnMetadata(
11641167
// stamp a default onto a column that predates that key — writing
11651168
// `includeTime: false` onto a legacy date column and truncating every
11661169
// stored time as a side effect of an unrelated metadata edit.
1167-
const merged: ColumnDefinition = { ...column, ...incoming }
1170+
// A `null` REMOVES its key rather than writing one — that is how a setting
1171+
// whose absence is meaningful (a `precision`, which absent means "render as
1172+
// stored") is returned to that state once set.
1173+
const sentKeys = Object.keys(incoming) as TypeSpecificColumnKey[]
1174+
const merged: ColumnDefinition = { ...column, ...metadataWithoutClears(incoming) }
1175+
for (const key of sentKeys) {
1176+
if (incoming[key] === null) delete merged[key]
1177+
}
11681178
const defaults = definition.defaultMetadata?.(merged) ?? {}
1169-
const sentKeys = new Set(Object.keys(incoming))
11701179
const updatedColumn: ColumnDefinition = {
11711180
...merged,
1172-
...pickMetadata(defaults, [...sentKeys] as TypeSpecificColumnKey[]),
1181+
// Defaults only fill keys this request SET; a cleared key stays cleared.
1182+
...metadataWithoutClears(
1183+
pickMetadata(
1184+
defaults,
1185+
sentKeys.filter((key) => incoming[key] !== null)
1186+
)
1187+
),
11731188
}
11741189

11751190
const columnValidation = validateColumnDefinition(updatedColumn)

0 commit comments

Comments
 (0)