Skip to content

Commit 13ab752

Browse files
committed
refactor(tables): fold json's draft handling into an editor variant
`json` differed from `text` in three modules by three separate `column.type === 'json'` branches — a monospace multi-line field, a pretty-printed expanded view, and a draft parsed on commit so a syntax error surfaces in the editor rather than being stored as text. Those three always travel together, so they are one editor variant, not three flags. `cleanCellValue`'s json/boolean/date branches and the expanded popover's date branches now key on the declared editor too, and the retype's convert-away-from-select check reads `storesOpaqueIds`. From the cleanup passes: "Decimal places" was labelled required when blank is its meaningful value; Badge's size-derived dot/icon maps were `Record<string,…>` so a new size half-added silently; the colour trigger had no hover affordance; and three comments were stale, duplicated, or claimed a test guarantee that did not exist — that last one is now true, with an assertion that co-owners of a shared metadata key agree on which writer handles it. The effect, state, memo, React Query and url-state passes found nothing to change. The callback pass found two pre-existing `useCallback`s in `table-filter.tsx` outside this change's lines; left alone rather than widening the diff. The number-spinner `inputClassName` was also left as-is — `.claude/rules/sim-styling.md` names it as sanctioned usage.
1 parent 3e850b2 commit 13ab752

11 files changed

Lines changed: 82 additions & 30 deletions

File tree

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,10 @@ function ColumnConfigBody({
133133
)
134134
// The RAW string, not a number. A numeric state round-tripped through
135135
// `Number()` on every keystroke cannot be cleared (`'' → 0`) and clamps
136-
// mid-typing (`1`, then `2` → `10`, never `12`). Parsing happens on commit,
137-
// matching `usage-limit-field`. Empty means "no precision declared", which is
138-
// what keeps a column rendering its values as stored.
136+
// mid-typing (`1`, then `2` → `10`, never `12`). The parse is derived below
137+
// and never written back, so the field keeps whatever was typed — same shape
138+
// as `usage-limit-field`. Empty means "no precision declared", which is what
139+
// keeps a column rendering its values as stored.
139140
const [precisionInput, setPrecisionInput] = useState<string>(() =>
140141
config.mode === 'edit' && existingColumn?.precision !== undefined
141142
? String(existingColumn.precision)
@@ -158,8 +159,6 @@ function ColumnConfigBody({
158159
// type that loses it cannot leave a stale control behind.
159160
const wantsCurrency = typeOwnsMetadataKey(typeInput, 'currencyCode')
160161
const wantsPrecision = typeOwnsMetadataKey(typeInput, 'precision')
161-
// `undefined` (blank field) is meaningful: it leaves the column rendering
162-
// values exactly as stored rather than forcing a decimal count.
163162
const parsedPrecision =
164163
precisionInput.trim() === '' ? undefined : clampPrecision(Number(precisionInput))
165164
const wantsIncludeTime = typeOwnsMetadataKey(typeInput, 'includeTime')
@@ -351,7 +350,9 @@ function ColumnConfigBody({
351350
<>
352351
<FieldDivider />
353352
<div className='flex flex-col gap-[9.5px]'>
354-
<RequiredLabel htmlFor='column-sidebar-precision'>Decimal places</RequiredLabel>
353+
<Label htmlFor='column-sidebar-precision' className='pl-0.5'>
354+
Decimal places
355+
</Label>
355356
<ChipInput
356357
id='column-sidebar-precision'
357358
type='number'

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) {
240240
// The one type wanting a mono multi-line field; `editor: 'text'` covers both
241241
// this and a plain input, so it stays explicit rather than inventing a field
242242
// only one type would ever set.
243-
if (column.type === 'json') {
243+
if (definition.editor === 'json') {
244244
return (
245245
<ChipModalField
246246
type='textarea'

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/select-field/select-color-picker.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export function SelectColorPicker({ color, onChange, optionName }: SelectColorPi
4242
return (
4343
<DropdownMenu>
4444
<DropdownMenuTrigger
45-
className='flex size-7 shrink-0 items-center justify-center'
45+
className='flex size-7 shrink-0 cursor-pointer items-center justify-center rounded-md transition-colors hover-hover:bg-[var(--surface-5)]'
4646
aria-label={`Color for ${optionName || 'option'}: ${labelFor(current)}`}
4747
>
4848
<Badge variant={current} size='swatch' />

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/cells/expanded-cell-popover.tsx

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,10 @@ export function ExpandedCellPopover({
7575
// stored value: a `duration` read as `5400` instead of `1:30:00`, a
7676
// `percent` as `25` instead of `25.0%`.
7777
//
78-
// `json` is the deliberate exception: its `formatForDisplay` is the
79-
// one-line form the grid cell needs, but the point of expanding a JSON cell
80-
// is to read it indented.
81-
if (target.column.type === 'json') return JSON.stringify(value, null, 2)
78+
// The `json` editor is the deliberate exception: its `formatForDisplay` is
79+
// the one-line form the grid cell needs, but the point of EXPANDING such a
80+
// cell is to read it indented.
81+
if (columnTypeOf(target.column).editor === 'json') return JSON.stringify(value, null, 2)
8282
return columnTypeOf(target.column).formatForDisplay(value, target.column)
8383
}, [target])
8484

@@ -159,7 +159,7 @@ export function ExpandedCellPopover({
159159
<ExpandedCellEditor
160160
key={`${expandedCell.rowId}:${expandedCell.columnKey ?? expandedCell.columnName}`}
161161
initialValue={
162-
target.column.type === 'date'
162+
columnTypeOf(target.column).editor === 'date'
163163
? storageToDisplay(formatValueForInput(target.value, target.column), {
164164
seconds: true,
165165
})
@@ -226,10 +226,13 @@ function ExpandedCellEditor({
226226
onClose()
227227
return
228228
}
229-
// Only date columns go through `displayToStorage` — it now parses many
230-
// date shapes, so a number draft like "2024" must not reach it.
229+
// Only date-editor columns go through `displayToStorage` — it parses many
230+
// date shapes, so a number draft like "2024" must not reach it. Keyed on
231+
// the editor the type declares, which is also what chose the format above.
231232
const raw =
232-
column.type === 'date' ? (displayToStorage(draftValue, timeZone) ?? draftValue) : draftValue
233+
columnTypeOf(column).editor === 'date'
234+
? (displayToStorage(draftValue, timeZone) ?? draftValue)
235+
: draftValue
233236
let cleaned: unknown
234237
try {
235238
cleaned = cleanCellValue(raw, column, timeZone)

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/utils.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,20 @@ export function cleanCellValue(
2626
column: ColumnDefinition,
2727
timeZone?: string
2828
): unknown {
29-
// These three read the browser's own context (the viewer's timezone, a JSON
30-
// draft that must throw so the editor can show a parse error, a checkbox's
31-
// truthiness) so they cannot come from the shared coercion.
32-
if (column.type === 'json') {
29+
// These three read the browser's own context (the viewer's timezone, a
30+
// structured draft that must THROW so the editor can show a parse error, a
31+
// checkbox's truthiness) so they cannot come from the shared coercion. Keyed
32+
// on the editor each type declares, not on its id.
33+
const editor = columnTypeOf(column).editor
34+
if (editor === 'json') {
3335
if (typeof value === 'string') {
3436
if (value === '') return null
3537
return JSON.parse(value)
3638
}
3739
return value
3840
}
39-
if (column.type === 'boolean') return Boolean(value)
40-
if (column.type === 'date') {
41+
if (editor === 'toggle') return Boolean(value)
42+
if (editor === 'date') {
4143
if (value === '' || value === null || value === undefined) return null
4244
return displayToStorage(String(value), timeZone)
4345
}

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ import {
1818
columnTypeById,
1919
isColumnType,
2020
isValueCompatible,
21+
ownersOfMetadataKey,
22+
TYPE_SPECIFIC_COLUMN_KEYS,
2123
} from '@/lib/table/column-types'
2224
import type { ColumnDefinition } from '@/lib/table/types'
2325
import { validateColumnDefinition } from '@/lib/table/validation'
@@ -85,6 +87,32 @@ describe('registry shape', () => {
8587
})
8688
})
8789

90+
describe('metadata key ownership', () => {
91+
it('has co-owners of a shared key agree on which writer handles it', () => {
92+
// `metadataKeysIn` reads the answer off ANY one owner, so co-owners that
93+
// disagreed would route the same key to different writers depending only
94+
// on registration order. `precision` is shared by `number` and `percent`.
95+
for (const key of TYPE_SPECIFIC_COLUMN_KEYS) {
96+
const owners = ownersOfMetadataKey(key)
97+
if (owners.length < 2) continue
98+
const handled = owners.map((o) =>
99+
[...(o.genericMetadataUpdate ?? o.ownedMetadata)].sort().join(',')
100+
)
101+
expect(new Set(handled).size, `owners of "${key}" disagree`).toBe(1)
102+
}
103+
})
104+
105+
it('lists an owner for every declared key, and only real types', () => {
106+
for (const key of TYPE_SPECIFIC_COLUMN_KEYS) {
107+
const owners = ownersOfMetadataKey(key)
108+
expect(owners.length, `"${key}" has no owner`).toBeGreaterThan(0)
109+
for (const owner of owners) {
110+
expect(owner.ownedMetadata).toContain(key)
111+
}
112+
}
113+
})
114+
})
115+
88116
describe('conversion write-back', () => {
89117
// A retype is allowed exactly when the target's `coerce` accepts the value,
90118
// and `coerce` often TRANSFORMS it. The conversion must therefore write the

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export const jsonColumnType: ColumnTypeDefinition = {
1313
sampleValue: 'value',
1414
ownedMetadata: ownedKeysOf('json'),
1515
workflowInputType: 'object',
16-
editor: 'text',
16+
editor: 'json',
1717
expandable: true,
1818

1919
coerce(value) {

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ export type ColumnCellEditor =
5757
| 'select'
5858
/** Not editable inline — the grid toggles it in place instead. */
5959
| 'toggle'
60+
/**
61+
* A structured document: monospace multi-line input, pretty-printed when
62+
* expanded, and a draft that is PARSED on commit so a syntax error surfaces
63+
* as an editor message instead of being stored as text.
64+
*
65+
* Its own variant rather than three separate flags, because those three
66+
* behaviours always travel together and were otherwise three separate
67+
* `column.type === 'json'` branches in three modules.
68+
*/
69+
| 'json'
6070

6171
/**
6272
* Optional `ColumnDefinition` keys that belong to a specific column type rather

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,7 @@ export async function updateColumnType(
748748
// Leaving `select` behind: stored cells hold option ids, which mean nothing
749749
// once the column is text/number/etc. Check compatibility against the option
750750
// NAME — that's what the cell will actually become (migrated below).
751-
const convertingAwayFromSelect = column.type === 'select' && !isSelectType
751+
const convertingAwayFromSelect = columnTypeOf(column).storesOpaqueIds && !isSelectType
752752
// The constraint the column ends up with, which may be arriving in this
753753
// same request — this write applies it, so the scan below has to judge
754754
// against the target value rather than the current one.

apps/sim/lib/table/query-builder/converters.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,8 +381,6 @@ export function filterRulesToPredicate(
381381
// A select value is an opaque option id — never scalar-coerce it (an id
382382
// that happens to look numeric would silently become a number and match
383383
// nothing). Same rule as filterRulesToFilter.
384-
// Opaque-id values must NOT be scalar-coerced: an option id of `"1"` would
385-
// become the number 1 and then match nothing under JSONB containment.
386384
const ruleColumn = columns.find((c) => columnMatchesRef(c, rule.column))
387385
const isSelect = ruleColumn ? columnTypeOf(ruleColumn).storesOpaqueIds : false
388386
current.push(ruleToPredicate(rule, isSelect))

0 commit comments

Comments
 (0)