diff --git a/.changeset/bulk-dialog-schema-aware-multiple.md b/.changeset/bulk-dialog-schema-aware-multiple.md new file mode 100644 index 000000000..81ce17b9a --- /dev/null +++ b/.changeset/bulk-dialog-schema-aware-multiple.md @@ -0,0 +1,25 @@ +--- +"@object-ui/plugin-grid": patch +--- + +fix(plugin-grid): schema-aware multi-value semantics for bulk-edit params (#2204) + +BulkActionDialog was schema-blind: whether a bulk-edit param rendered a +single- or multi-select — and whether the patch shipped a scalar or an +array — depended solely on the hand-written `BulkActionParam.multiple` +flag. A view author targeting a multi-value field (`multiselect`, `tags`, +`checkboxes`, or `select`/`lookup`/`user`/`file`/`image` with +`multiple: true`) who forgot the flag got a single-select control and a +SCALAR patch, silently corrupting the column shape server-side. + +Now the target object's schema is the fallback: + +- ObjectGrid passes its `objectSchema.fields` into BulkActionDialog and + useBulkExecutor. +- An explicit `param.multiple` boolean still wins; otherwise `update` + params derive multi-ness from the field definition via the new + `isMultiValueField` helper. +- The executor shape-normalizes every outgoing patch (`run` and `retry`): + a lone scalar aimed at a multi-value field is wrapped into a + single-element array — mirroring the server-side guard added in + framework #2552. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index daf9effac..e9e013ec0 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -2469,6 +2469,7 @@ export const ObjectGrid: React.FC = ({ onClose={handleBulkDialogClose} dataSource={dataSource as any} resource={schema.objectName ?? ''} + objectFields={objectSchema?.fields} /> ); diff --git a/packages/plugin-grid/src/__tests__/bulkActionSchemaMultiple.test.tsx b/packages/plugin-grid/src/__tests__/bulkActionSchemaMultiple.test.tsx new file mode 100644 index 000000000..12fc630e9 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/bulkActionSchemaMultiple.test.tsx @@ -0,0 +1,274 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Schema-aware multi-value semantics for bulk-edit params (#2204). + * + * The dialog used to be schema-blind: single/multi shape depended solely on + * the hand-written `BulkActionParam.multiple` flag. A view author targeting a + * multiselect field who forgot `multiple: true` got a single-select control + * and a SCALAR patch — silently corrupting the column shape server-side + * (until framework #2552 added a server-side wrap, the scalar was stored + * verbatim). Now the target object's schema is the fallback: param.multiple + * (explicit boolean) wins, otherwise `update` params derive multi-ness from + * the field definition, and the executor shape-normalizes every patch. + */ +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { renderHook, act } from '@testing-library/react'; + +import { BulkActionDialog } from '../components/BulkActionDialog'; +import { useBulkExecutor } from '../hooks/useBulkExecutor'; +import { isMultiValueField, normalizeMultiValuePatch } from '../hooks/multiValueFields'; + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } + if (!(Element.prototype as any).hasPointerCapture) { + (Element.prototype as any).hasPointerCapture = () => false; + } + if (!(Element.prototype as any).setPointerCapture) { + (Element.prototype as any).setPointerCapture = () => {}; + } +}); + +function makeDataSource() { + const update = vi.fn(async () => ({})); + const del = vi.fn(async () => ({})); + return { update, delete: del } as any; +} + +describe('isMultiValueField', () => { + it('recognizes inherently-multi types', () => { + expect(isMultiValueField({ type: 'multiselect' })).toBe(true); + expect(isMultiValueField({ type: 'checkboxes' })).toBe(true); + expect(isMultiValueField({ type: 'tags' })).toBe(true); + }); + + it('recognizes multiple-flagged select/lookup/user/file/image', () => { + for (const type of ['select', 'radio', 'lookup', 'user', 'file', 'image']) { + expect(isMultiValueField({ type, multiple: true })).toBe(true); + expect(isMultiValueField({ type })).toBe(false); + } + }); + + it('is false for single-value and unknown shapes', () => { + expect(isMultiValueField({ type: 'text' })).toBe(false); + expect(isMultiValueField({ type: 'text', multiple: true })).toBe(false); + expect(isMultiValueField(undefined)).toBe(false); + expect(isMultiValueField({})).toBe(false); + }); +}); + +describe('normalizeMultiValuePatch', () => { + const fields = { + labels: { type: 'multiselect' }, + team_members: { type: 'user', multiple: true }, + status: { type: 'select' }, + }; + + it('wraps scalars aimed at multi-value fields, leaves the rest alone', () => { + const patch = { labels: 'frontend', team_members: 'u1', status: 'active', note: 'x' }; + expect(normalizeMultiValuePatch(patch, fields)).toEqual({ + labels: ['frontend'], + team_members: ['u1'], + status: 'active', + note: 'x', + }); + // Input is not mutated. + expect(patch.labels).toBe('frontend'); + }); + + it('passes arrays/null through and no-ops without a schema', () => { + const patch = { labels: ['a', 'b'], team_members: null }; + expect(normalizeMultiValuePatch(patch, fields)).toBe(patch); + expect(normalizeMultiValuePatch({ labels: 'x' }, undefined)).toEqual({ labels: 'x' }); + }); +}); + +describe('BulkActionDialog — schema fallback when param.multiple is omitted (#2204)', () => { + it('renders the multi-select control and patches an ARRAY for a multiselect field', async () => { + const ds = makeDataSource(); + // NOTE: no `multiple` on the param — the pre-#2204 bug scenario. + const def: any = { + name: 'set_labels', + label: 'Set labels', + operation: 'update', + params: [ + { + name: 'labels', + label: 'Labels', + type: 'select', + required: true, + options: [ + { label: 'Frontend', value: 'frontend' }, + { label: 'Design', value: 'design' }, + ], + }, + ], + }; + render( + {}} + objectFields={{ labels: { type: 'multiselect' } }} + />, + ); + + // Schema says multi → the combobox (Popover multi-select) renders, not a + // single-select trigger that collapses to one value. + fireEvent.click(screen.getByRole('combobox')); + fireEvent.click(await screen.findByText('Frontend')); + fireEvent.click(await screen.findByText('Design')); + + const next = screen.getByRole('button', { name: 'Next' }); + await waitFor(() => expect(next).toBeEnabled()); + fireEvent.click(next); + fireEvent.click(await screen.findByRole('button', { name: 'Run' })); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(2)); + expect(ds.update).toHaveBeenCalledWith('showcase_project', 'r1', { labels: ['frontend', 'design'] }); + }); + + it('keeps single-select when the target field is single-value (non-regression)', () => { + const ds = makeDataSource(); + const def: any = { + name: 'set_status', + label: 'Set status', + operation: 'update', + params: [ + { + name: 'status', + label: 'Status', + type: 'select', + options: [{ label: 'Active', value: 'active' }], + }, + ], + }; + render( + {}} + objectFields={{ status: { type: 'select' } }} + />, + ); + // Single Select trigger renders (no multi Popover button with badges). + const trigger = screen.getByRole('combobox'); + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + // Radix Select trigger exposes data-slot/select semantics distinct from + // the multi control's outline Button; the reliable signal is that + // clicking does NOT render the Command search input used by multi-select. + fireEvent.click(trigger); + expect(document.querySelector('[cmdk-input]')).not.toBeInTheDocument(); + }); + + it('an explicit param.multiple=false wins over the schema, but the executor still ships an array', async () => { + const ds = makeDataSource(); + const def: any = { + name: 'set_primary_label', + label: 'Set primary label', + operation: 'update', + params: [ + { + name: 'labels', + label: 'Label', + type: 'select', + multiple: false, + options: [{ label: 'Frontend', value: 'frontend' }], + }, + ], + }; + render( + {}} + objectFields={{ labels: { type: 'multiselect' } }} + />, + ); + + // Author forced single-select UI… + fireEvent.click(screen.getByRole('combobox')); + fireEvent.click(await screen.findByText('Frontend')); + fireEvent.click(screen.getByRole('button', { name: 'Next' })); + fireEvent.click(await screen.findByRole('button', { name: 'Run' })); + + // …but the patch is still shape-normalized to an array for the + // multiselect column (mirrors the framework #2552 server behavior). + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update).toHaveBeenCalledWith('showcase_project', 'r1', { labels: ['frontend'] }); + }); +}); + +describe('useBulkExecutor — patch shape normalization (#2204)', () => { + it('wraps a scalar def.patch value aimed at a multi-value field', async () => { + const update = vi.fn(async () => ({})); + const ds = { update, delete: vi.fn() }; + const def: any = { + name: 'tag_frontend', + operation: 'update', + patch: { labels: 'frontend' }, + }; + const { result } = renderHook(() => + useBulkExecutor({ + resource: 'showcase_project', + dataSource: ds, + objectFields: { labels: { type: 'multiselect' } }, + }), + ); + + await act(async () => { + await result.current.run(def, [{ id: '1' }], {}); + }); + + expect(update).toHaveBeenCalledWith('showcase_project', '1', { labels: ['frontend'] }); + }); + + it('retry re-sends the normalized (array) patch', async () => { + let fail = true; + const update = vi.fn(async () => { + if (fail) throw new Error('boom'); + return {}; + }); + const ds = { update, delete: vi.fn() }; + const def: any = { name: 'tag', operation: 'update', patch: {} }; + const { result } = renderHook(() => + useBulkExecutor({ + resource: 'showcase_project', + dataSource: ds, + objectFields: { team_members: { type: 'user', multiple: true } }, + }), + ); + + await act(async () => { + await result.current.run(def, [{ id: '1' }], { team_members: 'u1' }); + }); + expect(result.current.result?.failed).toBe(1); + + fail = false; + update.mockClear(); + await act(async () => { + await result.current.retry('1'); + }); + expect(update).toHaveBeenCalledWith('showcase_project', '1', { team_members: ['u1'] }); + }); +}); diff --git a/packages/plugin-grid/src/components/BulkActionDialog.tsx b/packages/plugin-grid/src/components/BulkActionDialog.tsx index d8b348b4d..9fde2a39c 100644 --- a/packages/plugin-grid/src/components/BulkActionDialog.tsx +++ b/packages/plugin-grid/src/components/BulkActionDialog.tsx @@ -41,6 +41,7 @@ import { AlertTriangle, Check, CheckCircle2, ChevronsUpDown, Loader2, XCircle } import { useObjectTranslation } from '@object-ui/react'; import type { BulkActionDef, BulkActionParam } from '@object-ui/types'; import { useBulkExecutor, type BulkExecutorOptions, type BulkResult } from '../hooks/useBulkExecutor'; +import { isMultiValueField, type MultiValueFieldDef } from '../hooks/multiValueFields'; export interface BulkActionDialogProps { /** The action being executed. */ @@ -62,6 +63,15 @@ export interface BulkActionDialogProps { onClose: (result?: BulkResult | null) => void; /** Optional column to use as a row label in previews (defaults to 'name'). */ labelKey?: string; + /** + * Object-schema `fields` map for `resource`. Used to derive single- vs + * multi-value semantics for `update` params whose author did not declare + * `multiple` explicitly (#2204): a param aimed at a multiselect / tags / + * checkboxes field — or a select / lookup / user / file / image field + * flagged `multiple: true` — renders the multi-select control and patches + * an array, instead of silently degrading to single-select + scalar. + */ + objectFields?: Record; } type Step = 'params' | 'confirm' | 'running' | 'result'; @@ -84,9 +94,22 @@ export const BulkActionDialog: React.FC = ({ open, onClose, labelKey = 'name', + objectFields, }) => { const { t } = useObjectTranslation(); const params = def?.params ?? []; + + // Effective multi-value semantics for an `update` param: an explicit + // `param.multiple` wins; when the author omitted it, fall back to the + // target field's own schema so a multi-value column never silently gets + // a single-select control + scalar patch (#2204). Non-update operations + // pass params to a handler (not a field patch), so schema fallback does + // not apply there. + const isParamMultiple = useCallback((p: BulkActionParam): boolean => { + if (typeof p.multiple === 'boolean') return p.multiple; + if (def?.operation !== 'update') return false; + return isMultiValueField(objectFields?.[p.name]); + }, [def?.operation, objectFields]); const initialParamValues = useMemo>(() => { const v: Record = {}; for (const p of params) { @@ -99,7 +122,7 @@ export const BulkActionDialog: React.FC = ({ const [step, setStep] = useState('params'); const [values, setValues] = useState>(initialParamValues); const [lookupCache, setLookupCache] = useState>({}); - const { run, undo, retry, progress, result, reset } = useBulkExecutor({ resource, dataSource }); + const { run, undo, retry, progress, result, reset } = useBulkExecutor({ resource, dataSource, objectFields }); const [retrying, setRetrying] = useState(null); const [undoing, setUndoing] = useState(false); const [undoneAt, setUndoneAt] = useState(null); @@ -261,6 +284,7 @@ export const BulkActionDialog: React.FC = ({ setValues(prev => ({ ...prev, [p.name]: v }))} @@ -436,12 +460,14 @@ export const BulkActionDialog: React.FC = ({ interface ParamFieldProps { param: BulkActionParam; + /** Effective multi-value semantics — explicit `param.multiple` or the target field's schema (#2204). */ + multiple: boolean; value: unknown; onChange: (v: unknown) => void; lookupOptions?: LookupOption[]; } -const ParamField: React.FC = ({ param, value, onChange, lookupOptions }) => { +const ParamField: React.FC = ({ param, multiple, value, onChange, lookupOptions }) => { const { t } = useObjectTranslation(); const id = `bulk-param-${param.name}`; const label = ( @@ -474,7 +500,7 @@ const ParamField: React.FC = ({ param, value, onChange, lookupO break; case 'select': { const options = (param.options ?? []).map(o => ({ value: String(o.value), label: o.label })); - control = param.multiple ? ( + control = multiple ? ( = ({ param, value, onChange, lookupO const loadingPlaceholder = opts.length === 0 ? t('grid.bulk.loading', { defaultValue: 'Loading…' }) : (param.placeholder ?? t('grid.bulk.selectPlaceholder', { defaultValue: 'Select…' })); - control = param.multiple ? ( + control = multiple ? ( , + fields: Record | undefined | null, +): Record { + if (!fields) return patch; + let out: Record | null = null; + for (const [key, value] of Object.entries(patch)) { + if (value === null || value === undefined || Array.isArray(value)) continue; + if (!isMultiValueField(fields[key])) continue; + const t = typeof value; + if (t === 'string' || t === 'number' || t === 'boolean') { + if (!out) out = { ...patch }; + out[key] = [value]; + } + } + return out ?? patch; +} diff --git a/packages/plugin-grid/src/hooks/useBulkExecutor.ts b/packages/plugin-grid/src/hooks/useBulkExecutor.ts index 3bb616fe9..98084adec 100644 --- a/packages/plugin-grid/src/hooks/useBulkExecutor.ts +++ b/packages/plugin-grid/src/hooks/useBulkExecutor.ts @@ -10,6 +10,7 @@ import { useCallback, useRef, useState } from 'react'; import type { BulkActionDef } from '@object-ui/types'; import { RelatedCountStore } from '@object-ui/components'; import { executeBulkBatch } from '@object-ui/core'; +import { normalizeMultiValuePatch, type MultiValueFieldDef } from './multiValueFields'; export interface BulkRowError { id: string; @@ -44,6 +45,14 @@ export interface BulkSnapshotEntry { export interface BulkExecutorOptions { /** ObjectQL resource name (e.g. 'account'). */ resource: string; + /** + * Object-schema `fields` map for `resource`. When provided, patches built + * from `def.patch` + params are shape-normalized before hitting the data + * source: a lone scalar aimed at a multi-value field (multiselect / tags / + * checkboxes, or select / lookup / user / file / image with + * `multiple: true`) is wrapped into a single-element array (#2204). + */ + objectFields?: Record; /** * Minimal data-source surface required by the executor. Matches the public * shape of `@object-ui/data-objectstack`'s DataSource without importing it, @@ -93,7 +102,7 @@ export interface BulkExecutorOptions { * Soft (`succeeded < total`) shortfalls surface as a single aggregate * error entry per batch — see comments inline. */ -export function useBulkExecutor({ resource, dataSource }: BulkExecutorOptions) { +export function useBulkExecutor({ resource, dataSource, objectFields }: BulkExecutorOptions) { const [progress, setProgress] = useState({ total: 0, done: 0, @@ -133,10 +142,8 @@ export function useBulkExecutor({ resource, dataSource }: BulkExecutorOptions) { setResult(null); setProgress({ total, done: 0, failed: 0, inFlight: true }); - const buildPatch = (): Record => ({ - ...(def.patch ?? {}), - ...params, - }); + const buildPatch = (): Record => + normalizeMultiValuePatch({ ...(def.patch ?? {}), ...params }, objectFields); // Capture pre-mutation values for keys touched by the patch — only // for `update` operations. `delete` is irreversible from the client, @@ -230,7 +237,7 @@ export function useBulkExecutor({ resource, dataSource }: BulkExecutorOptions) { } return finalResult; }, - [resource, dataSource], + [resource, dataSource, objectFields], ); /** @@ -283,7 +290,10 @@ export function useBulkExecutor({ resource, dataSource }: BulkExecutorOptions) { if (!last) return false; const row = last.rows.find(r => String(r.id ?? '') === rowId); if (!row) return false; - const patch = { ...(last.def.patch ?? {}), ...last.params }; + const patch = normalizeMultiValuePatch( + { ...(last.def.patch ?? {}), ...last.params }, + objectFields, + ); try { switch (last.def.operation) { case 'delete': @@ -314,7 +324,7 @@ export function useBulkExecutor({ resource, dataSource }: BulkExecutorOptions) { return false; } }, - [resource, dataSource], + [resource, dataSource, objectFields], ); return { run, undo, retry, progress, result, reset };