fix(components): filter-builder renderer hands onChange the declared FilterGroup

.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.

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<string, unknown>) {
+  const C = ComponentRegistry.get('filter-builder') as React.ComponentType<any>;
+  return render(
+    <I18nProvider persistLanguage={false}>
+      <C {...props} />
+    </I18nProvider>,
+  );
+}
+
+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<FilterBuilderSchema, 'onChange'>),
+    });
+
+    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<NonNullable<FilterBuilderSchema['onChange']>>().parameter(0).toEqualTypeOf<FilterGroup>();
+
+    // @ts-expect-error — the form-style event the renderer used to send is not the declared argument.
+    const eventHandler: NonNullable<FilterBuilderSchema['onChange']> = (event: { target: { name?: string; value: unknown } }) => {
+      void event.target.value;
+    };
+    expect(typeof eventHandler).toBe('function');
+  });
+});

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<string> = 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 (