Address PR review feedback (#7119)

- preserve explicit nulls from source-owned conversion normalization
- normalize hooked values before select migration
- cover null and select conversion rewrites

Author: j15z

apps/sim/lib/table/column-types/extension-points.test.ts

@@ -69,6 +69,20 @@ describe('column type extension points', () => {
     ).toBe('stored-value')
   })
 
+  it('preserves an intentional null from source normalization', () => {
+    Object.assign(definition, {
+      valueForConversion: () => null,
+    })
+
+    expect(
+      valueForTypeConversion(
+        'stored-value',
+        { name: 'source', type: 'string' },
+        { name: 'target', type: 'number' }
+      )
+    ).toBeNull()
+  })
+
   it('lets a type own CSV import coercion', () => {
     Object.assign(definition, {
       coerceImport: (value: unknown) => `imported:${String(value)}`,

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

@@ -96,7 +96,8 @@ export function valueForTypeConversion(
   source: ColumnDefinition,
   target: ColumnDefinition
 ): JsonValue {
-  return columnTypeOf(source).valueForConversion?.(value, target) ?? value
+  const normalized = columnTypeOf(source).valueForConversion?.(value, target)
+  return normalized === undefined ? value : normalized
 }
 
 /** This type's own metadata errors; types carrying no metadata report none. */

apps/sim/lib/table/columns/retype-cell.test.ts

@@ -2,13 +2,25 @@
  * @vitest-environment node
  */
 
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
+import { COLUMN_TYPE_REGISTRY } from '@/lib/table/column-types'
 import { retypeCellRewrite } from '@/lib/table/columns/service'
 import type { ColumnDefinition } from '@/lib/table/types'
 
 const column = (over: Partial<ColumnDefinition>): ColumnDefinition =>
   ({ name: 'col', type: 'string', ...over }) as ColumnDefinition
 
+const sourceDefinition = COLUMN_TYPE_REGISTRY.string
+const originalValueForConversion = sourceDefinition.valueForConversion
+
+afterEach(() => {
+  if (originalValueForConversion === undefined) {
+    Reflect.deleteProperty(sourceDefinition, 'valueForConversion')
+    return
+  }
+  Object.assign(sourceDefinition, { valueForConversion: originalValueForConversion })
+})
+
 describe('retypeCellRewrite', () => {
   it('preserves an empty string the target type can hold', () => {
     // `''` is a real stored value: `coerceRowValues` keeps it for `string`, and
@@ -32,6 +44,29 @@ describe('retypeCellRewrite', () => {
     expect(retypeCellRewrite('true', column({ type: 'boolean' }))).toEqual({ value: true })
   })
 
+  it('writes back null produced by source normalization', () => {
+    Object.assign(sourceDefinition, { valueForConversion: () => null })
+
+    expect(
+      retypeCellRewrite('stored-value', column({ type: 'number' }), column({ type: 'string' }))
+    ).toEqual({ value: null })
+  })
+
+  it('coerces source-normalized values into select storage', () => {
+    Object.assign(sourceDefinition, { valueForConversion: () => 'Choice' })
+
+    expect(
+      retypeCellRewrite(
+        'stored-value',
+        column({
+          type: 'select',
+          options: [{ id: 'opt_choice', name: 'Choice' }],
+        }),
+        column({ type: 'string' })
+      )
+    ).toEqual({ value: 'opt_choice' })
+  })
+
   it('skips a cell whose stored value already matches the coercion', () => {
     expect(retypeCellRewrite('kept', column({ type: 'json' }))).toBeNull()
     expect(retypeCellRewrite(3, column({ type: 'json' }))).toBeNull()

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

@@ -777,6 +777,8 @@ export function retypeCellRewrite(
     ? valueForTypeConversion(value as JsonValue, source, target)
     : (value as JsonValue)
 
+  if (effective === null) return { value: null }
+
   if (!isValueCompatibleWithColumn(effective, target)) {
     // Incompatible non-blanks never reach here: the compatibility scan already
     // refused the whole conversion for them.
@@ -919,6 +921,7 @@ export async function updateColumnType(
       const isSelectType = data.newType === 'select'
       const targetOptions = data.options ?? column.options ?? []
       const targetMultiple = data.multiple ?? column.multiple
+      const sourceNormalizesConversion = columnTypeOf(column).valueForConversion !== undefined
       // Leaving `select` behind: stored cells hold option ids, which mean nothing
       // once the column is text/number/etc. Check compatibility against the option
       // NAME — that's what the cell will actually become (migrated below).
@@ -1035,9 +1038,7 @@ export async function updateColumnType(
         resolved: new Map<string, JsonValue>(),
       }
       await migrationFrom(column.type)?.(migrationContext)
-      if (isSelectType) {
-        await migrationTo(data.newType)?.(migrationContext)
-      } else {
+      if (!isSelectType || sourceNormalizesConversion) {
         let rewriteAfterId: string | undefined
         while (true) {
           const rows = await readColumnRetypePage(
@@ -1065,6 +1066,9 @@ export async function updateColumnType(
           if (rows.length < retypeScanBatchSize) break
         }
       }
+      if (isSelectType) {
+        await migrationTo(data.newType)?.(migrationContext)
+      }
 
       // A `unique` arriving with this retype is validated HERE, against the values
       // the conversion just wrote — not by the separate constraint write that