diff --git a/.changeset/record-picker-filter-select-option-type.md b/.changeset/record-picker-filter-select-option-type.md new file mode 100644 index 0000000000..e5ff0a7abf --- /dev/null +++ b/.changeset/record-picker-filter-select-option-type.md @@ -0,0 +1,5 @@ +--- +'@object-ui/fields': patch +--- + +The record picker's filter panel now sends the AUTHORED value of a picked `select` filter option instead of the control's stringified form. Radix `Select` speaks strings — options render as `String(opt.value)` and `onValueChange` returns that string — and the panel stored it as-is, so a filter option whose value is a number or a boolean queried `{ level: "1" }` against records storing `level: 1`. The user picked an option that plainly has records and got an empty list. This bit the `lookup_filters` auto-derivation in particular: `lookup_filters: [{ field: 'level', operator: 'in', value: [1, 2, 3] }]` derives options whose values keep the author's type (`LookupFilterDef.value` is `unknown`), so every non-string option in the picker was unfilterable. The control's string is now mapped back through `col.options` at the control boundary — the control speaks string, the payload keeps the authored type — reusing the `matchOptionValue` / `toControlValue` semantics `@object-ui/components` introduced for the standalone form's select (#3090), rather than coercing in `filterValuesToRecord`, which would have to guess whether `"1"` meant `1` or `"1"`. Options coming from the object schema are strings by spec and round-trip unchanged, and the `number` / `boolean` filter inputs (which already converted explicitly) are untouched — the `select` branch is lifted to their standard, resolving the three-way inconsistency (#3422). diff --git a/packages/fields/src/widgets/RecordPickerDialog.filterOptions.test.tsx b/packages/fields/src/widgets/RecordPickerDialog.filterOptions.test.tsx index 2d8ad3b5db..7b7929fb32 100644 --- a/packages/fields/src/widgets/RecordPickerDialog.filterOptions.test.tsx +++ b/packages/fields/src/widgets/RecordPickerDialog.filterOptions.test.tsx @@ -39,6 +39,11 @@ * keep winning over the schema (including the `in`-operator options * auto-derived from `lookupFilters`). They guard the precedence and the * non-leniency the fix chose, not a defect it repaired. + * + * The second suite in this file covers #3422 — what the panel DOWNLOADS once an + * option is picked. Having the right options to choose from (#3336) and sending + * the right value for the chosen one are the two halves of the same control, so + * they are pinned side by side; see that suite's own header for its directions. */ import React from 'react'; @@ -112,10 +117,14 @@ const phaseFilterColumn = { field: 'project_phase', label: 'Project Phase', type * `findBy…`: the filter bar only exists once the filter columns are resolved, * which in the LookupField path waits on the referenced object's schema fetch. */ -async function openFilterSelect(): Promise { +async function openFilterPanel(): Promise { const bar = await screen.findByTestId('record-picker-filter-bar'); fireEvent.click(bar.querySelector('button')!); - const panel = await screen.findByTestId('record-picker-filter-panel'); + return (await screen.findByTestId('record-picker-filter-panel')) as HTMLElement; +} + +async function openFilterSelect(): Promise { + const panel = await openFilterPanel(); const trigger = panel.querySelector('[role="combobox"]'); expect(trigger).toBeTruthy(); await act(async () => { @@ -332,6 +341,242 @@ describe('RecordPickerDialog filter panel — select options come from the schem }); }); +/** + * Record Picker FILTER PANEL — the picked option keeps its AUTHORED type + * (#3422). + * + * Radix `Select` speaks strings: options render as `String(opt.value)` and + * `onValueChange` returns that string. The panel used to store the string + * as-is, and `filterValuesToRecord`'s `select` branch passes its value through + * untouched — so a `lookup_filters`-derived option whose authored value is a + * number or a boolean queried `{ level: "1" }` against records storing + * `level: 1`. The user picked an option that plainly has records and got an + * empty list. + * + * The fix maps the control's string back through `col.options` at the control + * boundary (`matchOptionValue`, the #3090 semantics from + * `packages/components/src/renderers/form/option-value.ts`) — NOT a coercion + * in `filterValuesToRecord`, which would have to guess whether `"1"` meant `1` + * or `"1"`. + * + * ## Directions, decided before the run + * + * - **The two reproducers are RED before / GREEN after**: `$filter.level` was + * the string `"1"`, `$filter.active` the string `"false"`. They assert + * `toBe(1)` / `toBe(false)` plus an explicit `typeof`, because `"1" == 1` and + * a loose assertion would have passed on the broken build. + * - **Three cases are NO-OP pins, green before and after** (marked + * individually): schema-derived options are already strings and must + * round-trip byte-identical (the fix must not start morphing the path + * #3336 just made work); and the `number` / `boolean` filter inputs, which + * already converted explicitly, must keep doing exactly that — the + * three-branch inconsistency the issue flagged is resolved by lifting + * `select` up to them, not by touching them. + * - **The collision case documents a tie-break, not a repair.** Two options + * whose `String()` forms collide (`1` and `'1'`) cannot both be addressed by + * a string-speaking control — Radix sees two `SelectItem`s with the same + * `value`. The first match wins; the case pins that so the resolution is a + * stated rule rather than an accident of `Array.prototype.find`. + */ +describe('RecordPickerDialog filter panel — picked option keeps its authored type (#3422)', () => { + const tasks = [{ id: 't1', name: 'Recalibrate press', level: 1, active: false }]; + + function makeTaskDataSource() { + const find = vi.fn(async () => ({ data: tasks, total: tasks.length })); + const getObjectSchema = vi.fn(async () => ({ + name: 'tasks', + fields: { + name: { type: 'text', label: 'Name' }, + level: { type: 'number', label: 'Level' }, + active: { type: 'boolean', label: 'Active' }, + }, + highlightFields: ['name'], + })); + return { find, getObjectSchema } as any; + } + + /** The `$filter` of the most recent `find` call. */ + function lastFilter(ds: any): Record { + return ds.find.mock.calls[ds.find.mock.calls.length - 1][1].$filter; + } + + /** + * REPRODUCER (red before the fix). `lookup_filters` with an `in` array of + * numbers derives a select whose option values are numbers + * (`LookupFilterDef.value` is `unknown`, so the author's type survives into + * `options`). Picking one must query the number. + */ + it('sends the authored NUMBER for a lookupFilters-derived select option', async () => { + const ds = makeTaskDataSource(); + render( + {}} + dataSource={ds} + objectName="tasks" + onSelect={() => {}} + cellRenderer={getCellRenderer} + lookupFilters={[{ field: 'level', operator: 'in', value: [1, 2, 3] }]} + />, + ); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await openFilterSelect(); + + await act(async () => { + fireEvent.click(await screen.findByRole('option', { name: '1' })); + }); + + // Pre-fix this was the string "1"; `toBe` distinguishes them, `==` would not. + await waitFor(() => expect(lastFilter(ds).level).toBe(1)); + expect(typeof lastFilter(ds).level).toBe('number'); + }); + + /** REPRODUCER (red before the fix) — the boolean half of the same morph. */ + it('sends the authored BOOLEAN for a lookupFilters-derived select option', async () => { + const ds = makeTaskDataSource(); + render( + {}} + dataSource={ds} + objectName="tasks" + onSelect={() => {}} + cellRenderer={getCellRenderer} + lookupFilters={[{ field: 'active', operator: 'in', value: [true, false] }]} + />, + ); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await openFilterSelect(); + + await act(async () => { + fireEvent.click(await screen.findByRole('option', { name: 'false' })); + }); + + // Pre-fix this was the string "false" — which is TRUTHY, so the query asked + // for the opposite of what the user picked wherever the backend coerces. + await waitFor(() => expect(lastFilter(ds).active).toBe(false)); + expect(typeof lastFilter(ds).active).toBe('boolean'); + }); + + /** + * NO-OP PIN (green before and after). Schema-derived options are strings by + * spec (`SelectOptionSchema.value` is `string`), so the #3336 path must come + * out of the round-trip byte-identical — the fix restores the authored type, + * it does not invent one. + */ + it('round-trips a schema-derived string option unchanged', async () => { + const ds = makeDataSource(); + render( + {}} + dataSource={ds} + objectName="projects" + columns={[{ field: 'project_phase', label: 'Project Phase', type: 'select' }]} + onSelect={() => {}} + cellRenderer={getCellRenderer} + fieldsMeta={projectFields} + filterColumns={[phaseFilterColumn]} + />, + ); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await openFilterSelect(); + + await act(async () => { + fireEvent.click(await screen.findByRole('option', { name: '03 Manufacturing' })); + }); + + await waitFor(() => expect(lastFilter(ds).project_phase).toBe('manufacturing')); + expect(typeof lastFilter(ds).project_phase).toBe('string'); + }); + + /** + * TIE-BREAK PIN. `1` and `'1'` both render as the control string `"1"`, so + * the control cannot tell them apart — Radix receives two `SelectItem`s with + * the same `value` and React two children with the same key. The lookup + * resolves to the FIRST declared match, whichever entry was clicked. This + * documents the rule; it does not claim such an option list is usable. + */ + it('resolves a String() collision to the first declared option', async () => { + const ds = makeTaskDataSource(); + render( + {}} + dataSource={ds} + objectName="tasks" + onSelect={() => {}} + cellRenderer={getCellRenderer} + filterColumns={[ + { + field: 'level', + label: 'Level', + type: 'select', + options: [ + { label: 'Number one', value: 1 }, + { label: 'String one', value: '1' }, + ], + }, + ]} + />, + ); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await openFilterSelect(); + + // Clicking the SECOND entry still yields the first match — that is the + // documented consequence of a collision, not a bug in this assertion. + await act(async () => { + fireEvent.click(await screen.findByRole('option', { name: 'String one' })); + }); + + await waitFor(() => expect(lastFilter(ds).level).toBe(1)); + expect(typeof lastFilter(ds).level).toBe('number'); + }); + + /** + * NO-OP PIN (green before and after). The `number` and `boolean` filter + * inputs already converted explicitly (`Number(raw)` / `Boolean(checked)`); + * the issue flagged the three branches disagreeing, and the fix resolves that + * by lifting `select` to their standard — these two must be untouched. + */ + it('leaves the number and boolean filter inputs converting as before', async () => { + const ds = makeTaskDataSource(); + render( + {}} + dataSource={ds} + objectName="tasks" + onSelect={() => {}} + cellRenderer={getCellRenderer} + filterColumns={[ + { field: 'level', label: 'Level', type: 'number' }, + { field: 'active', label: 'Active', type: 'boolean' }, + ]} + />, + ); + + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + const panel = await openFilterPanel(); + + await act(async () => { + fireEvent.change(panel.querySelector('input[type="number"]')!, { target: { value: '2' } }); + }); + await waitFor(() => expect(lastFilter(ds).level).toBe(2)); + expect(typeof lastFilter(ds).level).toBe('number'); + + await act(async () => { + fireEvent.click(panel.querySelector('[role="checkbox"]')!); + }); + await waitFor(() => expect(lastFilter(ds).active).toBe(true)); + expect(typeof lastFilter(ds).active).toBe('boolean'); + }); +}); + describe('LookupField → picker filter panel wiring (#3336)', () => { const lookup = { name: 'project', diff --git a/packages/fields/src/widgets/RecordPickerDialog.tsx b/packages/fields/src/widgets/RecordPickerDialog.tsx index 708d774004..487d26288c 100644 --- a/packages/fields/src/widgets/RecordPickerDialog.tsx +++ b/packages/fields/src/widgets/RecordPickerDialog.tsx @@ -220,10 +220,72 @@ function resolveSchemaOptions( return translateOptions(objectName, fieldName, options); } +/** + * Option-value round-trip helpers for the filter panel's `select` input + * (#3422). + * + * Radix `Select` speaks strings only: an option renders as + * `value={String(opt.value)}` and `onValueChange` hands that same string back. + * A filter option's value, however, is whatever the metadata author wrote — + * `lookup_filters: [{ field: 'level', operator: 'in', value: [1, 2, 3] }]` + * derives options with NUMBER values (`LookupFilterDef.value` is `unknown`) — + * so writing the control's string straight into `$filter` queried + * `{ level: "1" }` against records storing `level: 1`, and the panel returned + * nothing for an option that plainly has records. + * + * The remap therefore happens at the CONTROL boundary, not in + * `filterValuesToRecord`: the control speaks string, the payload keeps the + * authored type. Coercing downstream would mean guessing whether `"1"` meant + * `1` or `"1"` — a guess the option list already answers exactly. + * + * Replicated from `matchOptionValue` / `toControlValue` in + * `packages/components/src/renderers/form/option-value.ts` (#3090), which + * solved the identical morph for the standalone form's select. Those two are + * module-private to `@object-ui/components` (its public barrel does not export + * them and the package publishes no deep subpath), so `@object-ui/fields` + * keeps its own copy of the four lines rather than widening another package's + * API to share them. + */ + +/** + * Stringify a value for a string-speaking control, preserving null/undefined + * (an absent value must stay absent, not become `"undefined"`). + */ +function toControlValue(value: unknown): string | undefined { + return value == null ? undefined : String(value); +} + +/** + * Map a control's string back to the authored option value, so a numeric / + * boolean / object option round-trips with its type intact. Falls back to the + * raw string when nothing matches (a stale value, or a column that declares no + * options) — the pre-#3422 behaviour, so unmatched paths change nothing. + * + * When two options share a `String()` form — `1` and `'1'`, or two object + * values that both print `[object Object]` — the FIRST match wins. Such a list + * is already unrenderable as a dropdown (Radix would receive two `SelectItem`s + * with the same `value`, and React two children with the same key), so this + * tie-break exists to be deterministic, not to make an ambiguous option list + * work. + */ +function matchOptionValue( + options: ReadonlyArray<{ value: unknown }> | undefined, + raw: string, +): unknown { + const hit = options?.find(o => String(o.value) === raw); + return hit ? hit.value : raw; +} + /** * Convert user-entered filter bar values into a $filter Record. * Each key is a field name, each value the user-entered value. * Empty/null values are ignored. + * + * Note the empty string is the "no filter on this field" sentinel here (and + * the `select` control's "nothing picked" value), so an option whose authored + * value is `''` cannot be expressed as an active filter. That predates #3422 + * and is unchanged by it — the round-trip above restores an option's TYPE, it + * does not redefine what counts as an empty selection. */ function filterValuesToRecord( values: Record, @@ -828,9 +890,11 @@ export function RecordPickerDialog({ return (
+ {/* The control speaks string; the stored filter value keeps the + authored option type (#3422 — see `matchOptionValue`). */}