diff --git a/.changeset/26382-filter-builder-onchange-group.md b/.changeset/26382-filter-builder-onchange-group.md new file mode 100644 index 00000000000..00da71ef1d5 --- /dev/null +++ b/.changeset/26382-filter-builder-onchange-group.md @@ -0,0 +1,12 @@ +--- +'@object-ui/components': patch +--- + +fix(components): the `filter-builder` renderer calls `onChange` with the `FilterGroup` that `FilterBuilderSchema.onChange` declares + +Clause-②: no +Metadata-protocol: none + +`FilterBuilderSchema.onChange` declares `(filter: FilterGroup) => void`, but the `filter-builder` renderer typed its own `onChange` as `(event: any) => void` and called it with a form-style `{ target: { name, value } }` event. A handler written from the declaration read `.logic` and `.conditions` off that event and got `undefined`, and the `any` kept the compiler from reporting the mismatch. The renderer now takes the schema's own `onChange` type and passes the group it holds, so the declaration is the contract on both sides. No in-repo host bound the event shape. A host that read `event.target.value` reads the argument itself instead. + +The builder's own rows also admit the opt-in `exists` / `notExists` operators, which a `FilterGroup` cannot carry. The renderer no longer forwards a host's `extraOperators` to the builder, since `FilterBuilderSchema` declares no such key, so the node never offers them. The `FilterGroup` zod mirror refuses them in an authored `value`. If a stored value that skipped that validation still carries one, the renderer does not call `onChange` and logs a console error naming the operator, instead of handing on a group the declared type does not describe. diff --git a/ui/packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx b/ui/packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx new file mode 100644 index 00000000000..1d9b4b4ca01 --- /dev/null +++ b/ui/packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx @@ -0,0 +1,146 @@ +/** + * 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. + */ + +/** + * The `filter-builder` renderer calls `onChange` with the shape + * `FilterBuilderSchema.onChange` declares: the `FilterGroup` itself (#26382). + * + * It used to call it with a form-style `{ target: { name, value } }` event + * while the schema declared `(filter: FilterGroup) => void`, and an `any` on + * the renderer's own `onChange` kept the compiler from seeing the gap. A + * handler written from the declaration read `.logic` / `.conditions` off the + * event and got `undefined`. + * + * The renderer is invoked DIRECTLY from the registry, as the sibling + * `inline-locale-label-read-sites.test.tsx` does, so the assertion reads what + * the renderer itself hands over and not what `SchemaRenderer` adds around it. + */ + +import * as React from 'react'; +import { describe, it, expect, expectTypeOf, vi } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { I18nProvider } from '@object-ui/i18n'; +import { ComponentRegistry } from '@object-ui/core'; +import type { FilterBuilderSchema, FilterGroup } from '@object-ui/types'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there the +// cold transform is billed to `hookTimeout`. See +// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). +import '../renderers'; + +const FIELDS = [ + { value: 'name', label: 'Name', type: 'text' }, + { value: 'age', label: 'Age', type: 'number' }, +]; + +const VALUE: FilterGroup = { + id: 'root', + logic: 'or', + conditions: [{ id: 'c1', field: 'name', operator: 'equals', value: 'Ada' }], +}; + +function renderDirect(props: Record) { + const C = ComponentRegistry.get('filter-builder') as React.ComponentType; + return render( + + + , + ); +} + +describe('filter-builder renderer hands onChange the declared FilterGroup (#26382)', () => { + it('calls onChange with the group, not with a { target } event', () => { + const onChange = vi.fn<(filter: FilterGroup) => void>(); + renderDirect({ + schema: { type: 'filter-builder', name: 'filters', fields: FIELDS, value: VALUE }, + onChange, + }); + + fireEvent.click(screen.getByRole('button', { name: /add filter/i })); + + expect(onChange).toHaveBeenCalledTimes(1); + const [group] = onChange.mock.calls[0]; + expect(group).not.toHaveProperty('target'); + expect(group.logic).toBe('or'); + expect(group.conditions.map((c) => c.id)[0]).toBe('c1'); + expect(group.conditions).toHaveLength(2); + expect(group.conditions[1]).toMatchObject({ field: 'name', operator: 'equals' }); + }); + + it('calls a handler authored on the node itself the same way', () => { + const onChange = vi.fn<(filter: FilterGroup) => void>(); + renderDirect({ + schema: { type: 'filter-builder', name: 'filters', fields: FIELDS, value: VALUE }, + // `SchemaRenderer` spreads the node's own keys as props, so a typed node + // that authors `onChange` reaches the renderer through the same prop. + ...({ onChange } satisfies Pick), + }); + + fireEvent.click(screen.getByRole('button', { name: /add filter/i })); + + expect(onChange.mock.calls[0][0].conditions).toHaveLength(2); + }); + + it('keeps a host-forwarded extraOperators out of the builder, so exists / notExists are never offered', async () => { + // `extraOperators` is the builder's only operator-granting prop, and what it + // grants is outside the FilterGroup that onChange declares. A host that + // forwards it through the node must not widen the dropdown. + renderDirect({ + schema: { type: 'filter-builder', name: 'filters', fields: FIELDS, value: VALUE }, + extraOperators: ['exists', 'notExists'], + onChange: vi.fn<(filter: FilterGroup) => void>(), + }); + + // The row's controls are Radix comboboxes in field / operator / value + // order, so index 1 is the operator. + fireEvent.keyDown(screen.getAllByRole('combobox')[1], { key: 'ArrowDown' }); + const labels = await waitFor(() => { + const options = screen.getAllByRole('option').map((o) => o.textContent); + expect(options, 'the operator dropdown did not open').toContain('Is empty'); + return options; + }); + expect(labels).not.toContain('Is set'); + expect(labels).not.toContain('Is not set'); + }); + + it('skips the emit, and names the operator on the console, for a stored row the FilterGroup cannot carry', () => { + const onChange = vi.fn<(filter: FilterGroup) => void>(); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + // Off contract on purpose: `exists` is the builder's opt-in operator, not + // a protocol one, and the zod mirror refuses it in an authored `value`. + // A stored value that skipped that validation is the one way in left. + const offContract = { + ...VALUE, + conditions: [{ id: 'c1', field: 'name', operator: 'exists' }], + }; + try { + renderDirect({ + schema: { type: 'filter-builder', name: 'filters', fields: FIELDS, value: offContract }, + onChange, + }); + + fireEvent.click(screen.getByRole('button', { name: /add filter/i })); + + expect(onChange).not.toHaveBeenCalled(); + const messages = consoleError.mock.calls.map((args) => String(args[0])); + expect(messages.filter((m) => m.startsWith('filter-builder: onChange not called'))).toHaveLength(1); + expect(messages.join('\n')).toMatch(/operator "exists", which is outside the protocol/); + } finally { + consoleError.mockRestore(); + } + }); + + it('pins the declared signature at the type level', () => { + expectTypeOf>().parameter(0).toEqualTypeOf(); + + // @ts-expect-error — the form-style event the renderer used to send is not the declared argument. + const eventHandler: NonNullable = (event: { target: { name?: string; value: unknown } }) => { + void event.target.value; + }; + expect(typeof eventHandler).toBe('function'); + }); +}); diff --git a/ui/packages/components/src/renderers/complex/filter-builder.tsx b/ui/packages/components/src/renderers/complex/filter-builder.tsx index 81896bd22b1..5645f25cbc2 100644 --- a/ui/packages/components/src/renderers/complex/filter-builder.tsx +++ b/ui/packages/components/src/renderers/complex/filter-builder.tsx @@ -13,28 +13,66 @@ import { useDisplayLocale } from '@object-ui/i18n'; // use of it): this repo has its OWN `resolveKeyedI18nLabel` over a DIFFERENT // vocabulary, and neither resolver accepts the other's shape. `schema.label` is // the spec's INLINE locale map — see `BaseSchema.label` (objectui#4580). -import { resolveI18nLabel as resolveInlineI18nLabel } from '@objectstack/spec/ui'; -import { FilterBuilder } from '../../custom/filter-builder'; +import { resolveI18nLabel as resolveInlineI18nLabel, VIEW_FILTER_OPERATORS } from '@objectstack/spec/ui'; +import { FilterBuilder, type FilterBuilderRowGroup } from '../../custom/filter-builder'; import type { RendererHostProps } from '../../lib/renderer-host-props'; +const PROTOCOL_OPERATOR_IDS: ReadonlySet = new Set(VIEW_FILTER_OPERATORS); + +/** + * Is every row of the builder's group on the protocol's operator vocabulary, + * i.e. is it the `FilterGroup` that `FilterBuilderSchema.onChange` declares? + * + * The builder's own row also admits the opt-in `exists` / `notExists`, which a + * `FilterGroup` cannot carry. This node never grants them: `FilterBuilderSchema` + * declares no `extraOperators`, and the renderer below keeps a host's + * `extraOperators` out of the builder. Its `value` is a `FilterGroup`, whose zod + * mirror refuses an operator outside the protocol. A group that fails here + * therefore came from a stored `value` that skipped that validation. + */ +function isAuthoredFilterGroup(group: FilterBuilderRowGroup): group is FilterBuilderRowGroup & FilterGroup { + return group.conditions.every((condition) => PROTOCOL_OPERATOR_IDS.has(condition.operator)); +} + ComponentRegistry.register('filter-builder', // `value` is declared because it is read by name: a host that binds the node // (a form field) forwards the group it holds, the same authored `FilterGroup` // the node's own `value` is. - ({ schema, className, onChange, ...props }: { schema: FilterBuilderSchema; className?: string; onChange?: (event: any) => void; value?: FilterGroup; [key: string]: unknown } & RendererHostProps) => { + // + // `onChange` is typed by the schema's own declaration and is called with the + // group (#26382). It was typed `(event: any) => void` and called with a + // form-style `{ target: { name, value } }` event, so a handler written from + // `FilterBuilderSchema.onChange` read `.logic` / `.conditions` off the event + // and got `undefined`, and the `any` kept the compiler from saying so. + // + // `extraOperators` is taken out of the host spread and never reaches + // `FilterBuilder`. It is the builder's only operator-granting prop, and what + // it grants (`exists` / `notExists`) is outside the `FilterGroup` that + // `onChange` declares. `FilterBuilderSchema` declares no such key, so a host + // that forwards one is off contract, and the builder offers only protocol + // operators. + ({ schema, className, onChange, extraOperators: _refusedExtraOperators, ...props }: { schema: FilterBuilderSchema; className?: string; onChange?: FilterBuilderSchema['onChange']; value?: FilterGroup; [key: string]: unknown } & RendererHostProps) => { // Read-time resolution against the display locale (objectui#4580 revised // Q1-A). `BaseSchema.label` accepts `string | I18nLabel`; rendering the map // straight into a text node THREW "Objects are not valid as a React child". const locale = useDisplayLocale(); - const handleChange = (value: any) => { - if (onChange) { - onChange({ - target: { - name: schema.name, - value: value, - }, - }); + const handleChange = (group: FilterBuilderRowGroup) => { + if (!onChange) return; + // No dropdown on this node offers an operator outside the protocol, so + // only a stored `value` that skipped its zod validation reaches this + // branch. Handing the group on would make the declared `FilterGroup` + // false, and a throw here would crash a click in the user's hands, so the + // emit is skipped and the console names the operator. + if (!isAuthoredFilterGroup(group)) { + const offending = group.conditions.find((condition) => !PROTOCOL_OPERATOR_IDS.has(condition.operator)); + console.error( + `filter-builder: onChange not called. A row holds the operator "${offending?.operator}", which is ` + + "outside the protocol's view-filter operators, so the FilterGroup that onChange hands over cannot carry it. " + + 'Fix the stored filter value.', + ); + return; } + onChange(group); }; return (