fix(app-shell): an unmapped dataset-filter operator is inert, not destructive

.changeset/9372-unmapped-operator-inert.md

@@ -0,0 +1,47 @@
+---
+'@object-ui/app-shell': patch
+---
+
+Stop the Studio dataset-filter bridge from ERASING a stored filter when an edit
+cannot be serialized (objectui#9372). Behaviour change, not just a fix: three more
+operators are now STORED where they used to be dropped.
+
+**The erase.** `groupToCondition` answers `undefined` both when the author CLEARED the
+filter and when nothing survived serialization, and the inspector — which commits on
+every change — treated the two the same. The host applies patches as
+`{ ...draft, ...patch }`, so that commit SET `dataset.filter` (or a `measure.filter`)
+to `undefined`, which is exactly the patch shape `objectChangePatch` uses deliberately
+to erase it. Nothing errored.
+
+Two ordinary gestures reached it. Switching the only condition's operator to one this
+bridge did not map — `notContains`, `startsWith`, `endsWith` and `between`, all four
+ordinary entries in this inspector's menu, none of them opt-in. And, needing no
+operator at all, simply BLANKING the value of the only row: an incomplete row is
+dropped by the same path, the last part goes with it, and the answer is `undefined`.
+
+**The fix, unconditional and ahead of any per-operator question.** The two meanings are
+now distinguished: a group that still holds rows commits NOTHING and the stored filter
+is left alone; only a group with no rows — Clear all, or the last row removed — still
+commits `undefined`, because that is the author's own gesture. An operator this bridge
+cannot express is therefore inert, whichever operators it maps.
+
+⚠️ Deliberately not "emit something anyway". A filter emitted in a spelling that means
+something else is worse than one that was dropped, so the unmapped arm still drops.
+
+**And three of the four are no longer unmapped.** `notContains`, `startsWith` and
+`endsWith` now serialize to the spec's own `$notContains` / `$startsWith` / `$endsWith`
+and read back as the operator the author picked. The comment calling them operators
+"this dialect genuinely cannot express" was stale: `FILTER_OPERATORS` carries all four.
+Each is backed by a conformance reading rather than a guess — the Filter Protocol's
+canonical `FILTER_TEXT_CASES` covers all three, the spec's declared-type door passes
+them over `text` and refuses them over `number` / `date` / `boolean`, and this builder
+offers them only on its text bucket.
+
+`between` stays unmapped, for a reason about this bridge rather than the vocabulary:
+the builder pads a half-typed pair with an empty bound and the spec's comparand door
+accepts `[1, '']`, so emitting it needs a both-bounds-present rule first. It is now
+unmapped and inert instead of unmapped and destructive.
+
+Forward note for anyone pinning stored filters: a dataset filter written by this
+version may carry `$notContains` / `$startsWith` / `$endsWith`, which an older
+app-shell reads as non-representable and degrades to "edit it in the Source tab".

packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.filterInert-9372.test.tsx

@@ -0,0 +1,116 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * The inspector does not COMMIT an erase (objectui#9372).
+ *
+ * ## Why this file exists next to the pure one
+ *
+ * `datasetFilterCondition.unmappedInert-9372.test.ts` pins the decision —
+ * {@link isClearedGroup} tells the author's CLEAR gesture apart from a
+ * serialization that produced nothing. A decision nobody consults is a channel
+ * with no reader, and the pure file cannot tell the difference: it would stay
+ * green with the guard sitting unused beside the old `onCommit(...)` call.
+ *
+ * This file drives the real component, with the real `FilterBuilder` mounted
+ * inside it, and watches the ONE thing that caused the data loss — the patch.
+ *
+ * ## The gesture, and why it is the value and not the operator menu
+ *
+ * The card's route is an operator pick, but the same defect is reachable by
+ * blanking the VALUE of the only row, with no operator involved at all: the
+ * incomplete-row `continue` drops it, the last part goes, and the commit is
+ * `undefined`. The host applies patches as `{ ...draft, ...patch }`, so that
+ * commit SETS `filter` to `undefined` — the patch shape `objectChangePatch`
+ * uses deliberately to erase it. Blanking a text input is also the one gesture
+ * that needs no Radix listbox interaction, so this pin holds without driving a
+ * select open in a headless DOM.
+ *
+ * ## The control
+ *
+ * "`onPatch` was not called" is also what an unopened popover, a mis-queried
+ * input and a dead handler all look like. So the same file types a REAL value
+ * through the same input and asserts the patch that produces — if that control
+ * stops firing, the absence below stops meaning anything.
+ */
+import { describe, it, expect, vi, afterEach, type Mock } from 'vitest';
+import { render, screen, fireEvent, cleanup } from '@testing-library/react';
+import '@testing-library/jest-dom';
+
+// Stub the catalog hooks so the inspector renders without a MetadataClient /
+// network, but with ONE text field so the filter popover has something to draw.
+vi.mock('./useDatasetFields', () => ({
+  useObjectOptions: () => ({ options: [], loading: false }),
+  useDatasetFieldCatalog: () => ({
+    relationships: [],
+    fieldOptions: [{ value: 'name', label: 'Name', type: 'text' }],
+    loading: false,
+  }),
+  useDatasetUsage: () => ({ reports: 0, dashboards: 0, loading: false }),
+  fieldTypeToDimensionType: (t: string) => (t === 'date' ? 'date' : 'string'),
+}));
+
+import { DatasetDefaultInspector } from './DatasetDefaultInspector';
+
+afterEach(cleanup);
+
+const baseProps = { type: 'dataset', name: 'sales', locale: 'en-US' as const };
+
+/** A dataset whose filter is ALREADY stored — the thing that got destroyed. */
+const draft = {
+  name: 'sales',
+  label: 'Sales',
+  object: 'opportunity',
+  include: [],
+  dimensions: [],
+  measures: [],
+  filter: { name: { $eq: 'acme' } },
+};
+
+/** The inspector's patch channel, typed as the component declares it. */
+type PatchSpy = Mock<(patch: Record<string, unknown>) => void>;
+const patchSpy = (): PatchSpy => vi.fn<(patch: Record<string, unknown>) => void>();
+
+/** Render, open the Scope filter popover, and hand back the row's value input. */
+function openScopeFilter(onPatch: PatchSpy) {
+  render(<DatasetDefaultInspector {...baseProps} draft={draft} onPatch={onPatch} readOnly={false} />);
+  // The trigger summarises the stored filter; seeing it at all is already a
+  // reading that `conditionToGroup` found the stored shape representable.
+  fireEvent.click(screen.getByText('1 condition'));
+  return screen.getByDisplayValue('acme') as HTMLInputElement;
+}
+
+describe('DatasetDefaultInspector — a filter edit that cannot be stored commits nothing (objectui#9372)', () => {
+  it('CONTROL: typing a real value still commits it, so the absence below is about the blank', () => {
+    const onPatch = patchSpy();
+    const input = openScopeFilter(onPatch);
+    fireEvent.change(input, { target: { value: 'contoso' } });
+    expect(onPatch).toHaveBeenCalledWith({ filter: { name: { $eq: 'contoso' } } });
+  });
+
+  it('THE DEFECT: blanking the only row\'s value does NOT patch `filter` to undefined', () => {
+    const onPatch = patchSpy();
+    const input = openScopeFilter(onPatch);
+    fireEvent.change(input, { target: { value: '' } });
+    // Before the repair this called `onPatch({ filter: undefined })`, which the
+    // host spreads over the draft — the stored filter destroyed, nothing shown
+    // to the author, and `JSON.stringify` then omits the key on save.
+    for (const [patch] of onPatch.mock.calls) {
+      expect(
+        patch,
+        'the inspector committed a patch carrying `filter`; if it is undefined, that ERASES the stored filter',
+      ).not.toHaveProperty('filter');
+    }
+  });
+
+  it('and the author\'s own CLEAR gesture still reaches the draft', () => {
+    // The other half: "Clear all" empties the group, which IS the clear
+    // gesture, and must still commit `undefined`. Without this the repair
+    // could have been a blanket "never commit undefined", stranding the filter
+    // an author asked to remove.
+    const onPatch = patchSpy();
+    render(<DatasetDefaultInspector {...baseProps} draft={draft} onPatch={onPatch} readOnly={false} />);
+    fireEvent.click(screen.getByText('1 condition'));
+    fireEvent.click(screen.getByText('Clear all'));
+    expect(onPatch).toHaveBeenCalledWith({ filter: undefined });
+  });
+});

packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx

@@ -34,7 +34,7 @@ import { InspectorComboField, type InspectorComboOption } from './InspectorCombo
 import { toFieldName } from '../previews/object-fields-io.js';
 import { formatMeasure } from '@object-ui/core';
 import { useDisplayLocale } from '@object-ui/i18n';
-import { conditionToGroup, groupToCondition, type FilterCondition } from './datasetFilterCondition.js';
+import { conditionToGroup, groupToCondition, isClearedGroup, type BuilderGroup, type FilterCondition } from './datasetFilterCondition.js';
 import {
   useObjectOptions,
   useDatasetFieldCatalog,
@@ -244,6 +244,31 @@ function DatasetFilterField({ label, help, value, onCommit, fields, disabled }:
 }) {
   const { group, representable } = conditionToGroup(value);
   const count = group.conditions.length;
+  /**
+   * Commit an edit — unless nothing survived serialization while rows are
+   * still on screen (objectui#9372).
+   *
+   * `groupToCondition` answers `undefined` both when the author CLEARED the
+   * filter and when every row was dropped, and this commit is what turns the
+   * second one into data loss: `onCommit` lands as `onPatch({ filter })`, the
+   * host applies it as `{ ...draft, ...patch }`, so `filter` is SET to
+   * `undefined` — the very patch shape `objectChangePatch` uses to erase it.
+   * An unmapped operator (`between`) or a blanked value on the only row would
+   * therefore destroy a working stored filter, silently.
+   *
+   * Holding the patch leaves the stored value alone, which is the whole
+   * requirement. ⛔ It is deliberately not "emit something anyway": a filter in
+   * a spelling that means something else is worse than one that was dropped.
+   * ⚠️ Known and accepted: the builder re-seeds its own state from `value`
+   * whenever the two differ, so an unexpressible row is lost from the panel on
+   * the next render the inspector happens to do. Losing an edit the bridge
+   * could never have stored is not in the same class as destroying one it had.
+   */
+  const commitFilterGroup = (g: BuilderGroup) => {
+    const next = groupToCondition(g);
+    if (next === undefined && !isClearedGroup(g)) return;
+    onCommit(next);
+  };
   return (
     <div className="space-y-1">
       <Label className="text-xs text-muted-foreground">{label}</Label>
@@ -263,7 +288,7 @@ function DatasetFilterField({ label, help, value, onCommit, fields, disabled }:
             {fields.length === 0 ? (
               <p className="text-xs text-muted-foreground">Pick a base object to add filter conditions.</p>
             ) : (
-              <FilterBuilder fields={fields as any} value={group as any} onChange={(g: any) => onCommit(groupToCondition(g))} />
+              <FilterBuilder fields={fields as any} value={group as any} onChange={commitFilterGroup} />
             )}
           </PopoverContent>
         </Popover>

packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.nullOperators-9363.test.ts

@@ -108,13 +108,14 @@ describe('groupToCondition — the null predicates this inspector offers (object
   });
 
   it('still drops an operator it does not map, rather than emitting a wrong filter', () => {
-    // Deliberate, and kept: see this file's header. These four are OFFERED by
-    // the menu and dropped, which erases the same way — tracked as its own
-    // finding, not widened here on the way past.
-    expect(groupToCondition(row('notContains', 'a'))).toBeUndefined();
+    // Deliberate, and kept: see this file's header.
+    //
+    // objectui#9372 took the other three of the four this listed — the
+    // `notContains` / `startsWith` / `endsWith` rows are asserted as EMITTED
+    // in `datasetFilterCondition.unmappedInert-9372`, with the conformance
+    // reading behind each — and made the remaining drop inert. `between` is
+    // what is left: still offered, still dropped, and no longer destructive.
     expect(groupToCondition(row('between', [1, 5]))).toBeUndefined();
-    expect(groupToCondition(row('startsWith', 'a'))).toBeUndefined();
-    expect(groupToCondition(row('endsWith', 'a'))).toBeUndefined();
   });
 
   it('an empty group is still `undefined` — that is the author CLEARING the filter', () => {
@@ -191,15 +192,21 @@ const OFFERED = offeredAcrossBuckets([]);
 /**
  * Offered, and deliberately NOT expressible by this bridge today.
  *
- * Each one drops on commit, and a drop of the last surviving row erases the
- * stored filter — the same mechanism objectui#9363 fixed for the null pair.
- * They are listed rather than fixed here because mapping them is a separate
- * decision per operator (`between` needs a both-bounds-present rule before it
- * can be emitted at all), and a blanket "stop dropping" would emit filters that
- * mean something else. Mapping one is what makes this list shrink — and this
- * assertion go red until it is updated.
+ * Each one drops on commit. ⚠️ That drop used to ERASE the stored filter when
+ * no other row survived — the same mechanism objectui#9363 fixed for the null
+ * pair — and objectui#9372 ended that: the caller now tells "nothing survived"
+ * apart from "the author cleared", so a drop is inert
+ * (`datasetFilterCondition.unmappedInert-9372`). Being on this list is now a
+ * missing capability, not data loss.
+ *
+ * objectui#9372 also took three of the four this listed. `between` is what
+ * remains, and it remains for a reason that is about THIS bridge rather than
+ * the spec's vocabulary: the builder pads a half-typed pair with `''` and the
+ * spec's comparand door accepts `[1, '']`, so it needs a both-bounds-present
+ * rule before it can be emitted at all. Mapping it is what makes this list
+ * shrink — and this assertion go red until it is updated.
  */
-const DECLARED_UNEXPRESSIBLE = ['between', 'endsWith', 'notContains', 'startsWith'];
+const DECLARED_UNEXPRESSIBLE = ['between'];
 
 /** A value that keeps a row from being dropped as INCOMPLETE, per operator. */
 function probeValue(operator: string): unknown {

packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.test.ts

@@ -21,7 +21,12 @@ describe('datasetFilterCondition', () => {
   });
 
   it('drops unmapped operators rather than emitting a bad filter', () => {
-    expect(groupToCondition({ logic: 'and', conditions: [{ field: 'x', operator: 'notContains', value: 'a' }] }))
+    // The claim is unchanged; the FIXTURE moved. `notContains` stopped being
+    // an unmapped operator in objectui#9372 (it is bridged to `$notContains`,
+    // asserted there), so keeping it here would have pinned a branch it no
+    // longer reaches — an assertion that passes because nothing is produced.
+    // `between` is the operator this bridge still declines to emit.
+    expect(groupToCondition({ logic: 'and', conditions: [{ field: 'x', operator: 'between', value: [1, 5] }] }))
       .toBeUndefined();
   });