fix(app-shell): inspectors can block Save — the field inspector gates on CEL errors (#4306)

.changeset/inspector-blocking-issues-save-gate-4306.md

@@ -0,0 +1,15 @@
+---
+'@object-ui/app-shell': minor
+---
+
+Inspectors can block Save — a formula that does not parse no longer saves and publishes as the live field definition
+
+The field inspector rendered its CEL verdict and did nothing with it. Typing `record.est_hours *` into a formula field showed the inline parse error, left "Save draft" enabled, saved with a 200 and a success toast, and publishing made the malformed expression the live field definition. The RLS policy editor on the same build refused the same class of input — the inconsistency was inside one console, and the difference was structural rather than an oversight in one file.
+
+`CelPredicateField` has always reported its findings upward through `onLintChange`, but only `PermissionAdvancedFacets` listened, and both ends of that channel live inside the permission editor, which owns its own Save button. The field inspector is a registry component: the button it needed to gate belongs to a host — the Studio Data pillar or the metadata editor — reached only through `MetadataInspectorProps`, which had no way to say "what I am showing is not saveable". So the inspector had no channel to be wired to, and adding one is the fix.
+
+`MetadataInspectorProps` gains an optional `onBlockingIssuesChange(count)`. It is named for blocking issues rather than for CEL because the same gap reaches five more inspectors through `ConditionBuilder` and `ConditionalFormattingEditor`; those are wired separately, against this contract rather than a renamed one. The prop is optional, so every existing inspector remains valid and any inspector with nothing to block on simply never calls it.
+
+`ObjectFieldInspector` now aggregates its four CEL editors — the formula box and the `visibleWhen` / `readonlyWhen` / `requiredWhen` rules — into a per-site map rather than one running total. Four editors lint independently and asynchronously, so a shared counter would let whichever reported last overwrite the others: fixing one of two broken rules would hand back a writable Save while the other rule was still malformed. Two stale-verdict cases are settled by deriving the total rather than by repairing it afterwards, which leaves no window in which Save is gated by an editor that is already gone: the map is stamped with the field it describes, so a verdict that lands after the selection moved cannot gate the field now on screen, and the formula site is only counted while the field actually is a formula — otherwise typing a bad formula and switching the type away would wedge Save shut with no editor left to fix it in.
+
+Both hosts hold the count and disable their own Save, reusing the message the RLS editor already shows ("Fix the CEL syntax errors before saving.") so one console says one thing. Each host stamps the count with the selection it came from, so it expires when the selection changes or the panel closes — an unmounted inspector can never retract its last verdict, and a host that waited for one would leave Save permanently disabled. In the metadata editor all three save doors are gated, not just the button: the autosave timer and the ⌘S shortcut would otherwise have written the malformed definition a second later.

packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx

@@ -499,6 +499,17 @@ function MetadataResourceEditPageImpl({
   React.useEffect(() => {
     setSelection(null);
   }, [type, name]);
+  // Blocking author-time issues reported by the scoped inspector (e.g. a CEL
+  // formula that does not parse) — Save must refuse them rather than publish a
+  // malformed definition (objectui#4306).
+  //
+  // The count is STAMPED with the selection it describes, so it expires by
+  // construction when the selection changes or the inspector unmounts: a
+  // component that has gone away cannot retract its last verdict, and a host
+  // that waited for one would wedge Save shut.
+  const [blockingReport, setBlockingReport] = React.useState({ key: '', count: 0 });
+  const selectionKey = selection ? `${type}:${name}:${selection.kind}:${selection.id}` : '';
+  const inspectorBlocking = blockingReport.key === selectionKey ? blockingReport.count : 0;
   React.useEffect(() => {
     if (!editing) setSelection(null);
   }, [editing]);
@@ -1351,6 +1362,9 @@ function MetadataResourceEditPageImpl({
   React.useEffect(() => {
     if (!autoSaveEnabled) return;
     if (createMode || readOnly || !editing || !isDirty || saving) return;
+    // Autosave is a save door like any other: gating only the button would let
+    // the timer publish the malformed definition a second later (objectui#4306).
+    if (inspectorBlocking > 0) return;
     let snap: string;
     try {
       snap = JSON.stringify(draft);
@@ -1363,7 +1377,7 @@ function MetadataResourceEditPageImpl({
       doSaveRef.current(false);
     }, AUTOSAVE_DEBOUNCE_MS);
     return () => window.clearTimeout(handle);
-  }, [draft, isDirty, editing, saving, createMode, readOnly, autoSaveEnabled]);
+  }, [draft, isDirty, editing, saving, createMode, readOnly, autoSaveEnabled, inspectorBlocking]);
 
   // Keyboard shortcut — ⌘S / Ctrl+S triggers save when dirty.
   React.useEffect(() => {
@@ -1372,14 +1386,16 @@ function MetadataResourceEditPageImpl({
         if (!canWrite || readOnly) return;
         if (!editing && !createMode) return;
         e.preventDefault();
-        if (!saving && (createMode || isDirty)) {
+        // Third save door — the shortcut must respect the same gate as the
+        // button and the autosave timer (objectui#4306).
+        if (!saving && (createMode || isDirty) && inspectorBlocking === 0) {
           doSaveRef.current(false);
         }
       }
     };
     window.addEventListener('keydown', handler);
     return () => window.removeEventListener('keydown', handler);
-  }, [canWrite, readOnly, editing, createMode, saving, isDirty]);
+  }, [canWrite, readOnly, editing, createMode, saving, isDirty, inspectorBlocking]);
 
   // Beforeunload guard — browser-native "leave site?" prompt when the
   // user closes the tab / reloads with unsaved changes.
@@ -1720,14 +1736,16 @@ function MetadataResourceEditPageImpl({
         <Button
           size="sm"
           onClick={() => doSave(false)}
-          disabled={saving || (!createMode && !isDirty)}
+          disabled={saving || (!createMode && !isDirty) || inspectorBlocking > 0}
           className="h-7 w-7 p-0 relative"
           title={
             saving
               ? t('engine.edit.saving', locale)
-              : !createMode && !isDirty
-                ? t('engine.edit.noChanges', locale)
-                : `${t('engine.edit.save', locale)} (⌘S)`
+              : inspectorBlocking > 0
+                ? t('perm.cel.saveBlocked', locale)
+                : !createMode && !isDirty
+                  ? t('engine.edit.noChanges', locale)
+                  : `${t('engine.edit.save', locale)} (⌘S)`
           }
         >
           {saving ? (
@@ -2312,6 +2330,9 @@ function MetadataResourceEditPageImpl({
                             }
                             onClearSelection={() => setSelection(null)}
                             onSelectionChange={setSelection}
+                            onBlockingIssuesChange={(count) =>
+                              setBlockingReport({ key: selectionKey, count })
+                            }
                             readOnly={formReadOnly}
                             locale={locale}
                           />

packages/app-shell/src/views/metadata-admin/inspector-registry.ts

@@ -43,6 +43,21 @@ export interface MetadataInspectorProps {
    * that need to redirect the focused sub-element.
    */
   onSelectionChange?: (next: MetadataSelection | null) => void;
+  /**
+   * Report how many BLOCKING author-time issues the inspector is currently
+   * showing — e.g. a CEL expression that does not parse (objectui#4306).
+   *
+   * The host owns Save, so only the host can refuse to write; an inspector that
+   * renders a fault it cannot act on is how a malformed formula got saved and
+   * published as the live field definition. Fires whenever the aggregate
+   * changes, `0` when everything is clean.
+   *
+   * Optional — an inspector with nothing to block on simply never calls it.
+   * Hosts must reset their own count when the selection changes or the
+   * inspector unmounts rather than waiting for a final `0`, since a component
+   * that has gone away cannot report anything.
+   */
+  onBlockingIssuesChange?: (count: number) => void;
   /** Whether the host is in edit mode. False → disable inputs. */
   readOnly: boolean;
   /** Active UI locale for i18n. */

packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.celGate.test.tsx

@@ -0,0 +1,238 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * The field inspector must REPORT its blocking CEL verdicts upward, so the host
+ * that owns Save can refuse to publish a parse fault — objectui#4306.
+ *
+ * The card: `record.est_hours *` in the formula box shows its inline error and
+ * still saves (PUT 200 + toast), and publishing makes the malformed expression
+ * the live field definition. The RLS editor one directory over already gates
+ * correctly; the inspector simply reported nothing.
+ *
+ * What is pinned here is the INSPECTOR half of that channel — the per-site
+ * aggregation. The count is keyed by SITE (`formula` + the three conditional
+ * rules) rather than summed into one number at the mount, because four editors
+ * report independently and a shared counter would let whichever linted last
+ * overwrite the others: the classic multi-source aggregation bug. So the
+ * decisive case here is not "an error is counted" but
+ * {@link https://github.com/objectstack-ai/objectui/issues/4306 two sites at
+ * once, one of them clearing} — a shared counter passes every single-site case
+ * and fails that one.
+ *
+ * The host half (Save actually going disabled, and the host resetting its own
+ * count on selection change) is pinned in
+ * {@link file://../../studio-design/DataPillar.celGate.test.tsx}.
+ *
+ * The engine is stubbed deterministically — the live lint against the REAL
+ * engine is CelPredicateField.test.tsx's job; here we test wiring, so the
+ * stub treats "ends in a dangling binary operator" as the parse fault.
+ */
+
+import '@testing-library/jest-dom/vitest';
+import * as React from 'react';
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react';
+
+vi.mock('../useMetadata', () => ({
+  useMetadataClient: () => ({
+    list: vi.fn().mockResolvedValue([]),
+    listDrafts: vi.fn().mockResolvedValue([]),
+  }),
+}));
+
+vi.mock('../previews/useObjectFields', () => ({
+  useObjectFields: () => ({ fields: [], loading: false, error: null }),
+}));
+
+import { ObjectFieldInspector } from './ObjectFieldInspector';
+import { __setCelFormulaLoader } from '../celAuthoring';
+
+afterEach(() => {
+  cleanup();
+  __setCelFormulaLoader(undefined);
+});
+
+/** The card's repro shape: a source left hanging on a binary operator. */
+const DANGLING = /[*+\-/&|=<>]\s*$/;
+
+function stubEngine() {
+  __setCelFormulaLoader(() =>
+    Promise.resolve({
+      validateExpression: (_role: string, input: unknown) => {
+        const src = typeof input === 'string' ? input : String((input as { source?: string })?.source ?? '');
+        return DANGLING.test(src)
+          ? { ok: false, errors: [{ message: 'Parse error: expression ends after an operator' }], warnings: [] }
+          : { ok: true, errors: [], warnings: [] };
+      },
+      introspectScope: () => ({
+        fields: ['est_hours', 'status'],
+        roots: ['record', 'previous', 'parent'],
+        functions: ['has'],
+      }),
+      inferExpressionType: () => 'number' as const,
+    }),
+  );
+}
+
+function controlFor(label: string): HTMLElement {
+  const lab = screen.getByText(label);
+  return lab.parentElement!.querySelector('input, textarea, select, [role="combobox"]') as HTMLElement;
+}
+
+/**
+ * Stateful harness — the editors are controlled, so edits must round-trip
+ * through the draft or the second keystroke reverts the first.
+ */
+function Harness({
+  initialFields,
+  selected,
+  onBlockingIssuesChange,
+}: {
+  initialFields: Record<string, Record<string, unknown>>;
+  selected: string;
+  onBlockingIssuesChange: (count: number) => void;
+}) {
+  const [fields, setFields] = React.useState(initialFields);
+  return (
+    <ObjectFieldInspector
+      type="object"
+      name="account"
+      draft={{ name: 'account', fields }}
+      selection={{ kind: 'field', id: selected }}
+      onPatch={(patch: Record<string, unknown>) => setFields(patch.fields as typeof fields)}
+      onClearSelection={() => {}}
+      onSelectionChange={() => {}}
+      readOnly={false}
+      locale={'en-US'}
+      onBlockingIssuesChange={onBlockingIssuesChange}
+    />
+  );
+}
+
+function renderHarness(
+  initialFields: Record<string, Record<string, unknown>>,
+  selected: string,
+) {
+  const report = vi.fn();
+  render(
+    <Harness initialFields={initialFields} selected={selected} onBlockingIssuesChange={report} />,
+  );
+  /** The count the host would currently be holding. */
+  const current = () => report.mock.calls.at(-1)?.[0] as number | undefined;
+  return { report, current };
+}
+
+describe('ObjectFieldInspector — blocking CEL issues are reported to the host (#4306)', () => {
+  it("counts the card's own repro: a dangling operator in the formula box", async () => {
+    stubEngine();
+    const { current } = renderHarness(
+      { est_hours: { type: 'number' }, total: { type: 'formula' } },
+      'total',
+    );
+    fireEvent.change(controlFor('Formula (CEL)'), { target: { value: 'record.est_hours *' } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+  });
+
+  it('reports a clean formula as zero, so a valid expression never blocks Save', async () => {
+    stubEngine();
+    const { current } = renderHarness(
+      { est_hours: { type: 'number' }, total: { type: 'formula' } },
+      'total',
+    );
+    fireEvent.change(controlFor('Formula (CEL)'), { target: { value: 'record.est_hours * 2' } });
+    await waitFor(() => expect(current()).toBe(0), { timeout: 3000 });
+  });
+
+  it('re-enables Save when the author fixes the formula', async () => {
+    stubEngine();
+    const { current } = renderHarness({ total: { type: 'formula' } }, 'total');
+    const box = controlFor('Formula (CEL)');
+    fireEvent.change(box, { target: { value: 'record.est_hours *' } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+    fireEvent.change(box, { target: { value: 'record.est_hours * 2' } });
+    await waitFor(() => expect(current()).toBe(0), { timeout: 3000 });
+  });
+
+  it.each([
+    ['Visible when', 'visibleWhen'],
+    ['Read-only when', 'readonlyWhen'],
+    ['Required when', 'requiredWhen'],
+  ])('gates on a fault in the %s rule editor', async (label) => {
+    stubEngine();
+    const { current } = renderHarness({ note: { type: 'text' } }, 'note');
+    fireEvent.change(controlFor(label), { target: { value: "record.status ==" } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+  });
+
+  /**
+   * The decisive case for the per-site map. With one shared counter the last
+   * reporter wins: clearing ONE of two faulty editors would drop the total to
+   * 0 and hand back a Save button that still publishes the other fault.
+   */
+  it('keeps each site independent — clearing one fault leaves the other counted', async () => {
+    stubEngine();
+    const { current } = renderHarness({ note: { type: 'text' } }, 'note');
+    fireEvent.change(controlFor('Visible when'), { target: { value: 'record.status ==' } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+
+    fireEvent.change(controlFor('Read-only when'), { target: { value: 'record.amount >' } });
+    await waitFor(() => expect(current()).toBe(2), { timeout: 3000 });
+
+    // Fix only the first — the second must still hold Save closed.
+    fireEvent.change(controlFor('Visible when'), { target: { value: "record.status == 'open'" } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+  });
+
+  /**
+   * Prune on field-type change (ruling item 2): the formula editor UNMOUNTS
+   * when the field stops being a formula, so its last verdict must not
+   * survive — otherwise Save wedges closed with no editor on screen to fix.
+   */
+  it('prunes the formula verdict when the field stops being a formula', async () => {
+    stubEngine();
+    const report = vi.fn();
+    const current = () => report.mock.calls.at(-1)?.[0] as number | undefined;
+
+    // The Type control is a Radix combobox, not a native select, so the type
+    // change is driven at the draft level — which is what the prune actually
+    // keys on. The malformed `expression` deliberately STAYS on the field: the
+    // wedge this guards against is the stored fault outliving its editor.
+    function TypeSwitchHarness() {
+      const [fields, setFields] = React.useState<Record<string, Record<string, unknown>>>({
+        total: { type: 'formula' },
+      });
+      return (
+        <>
+          <button
+            type="button"
+            onClick={() =>
+              setFields((f) => ({ total: { ...f.total, type: 'text' } }))
+            }
+          >
+            retype as text
+          </button>
+          <ObjectFieldInspector
+            type="object"
+            name="account"
+            draft={{ name: 'account', fields }}
+            selection={{ kind: 'field', id: 'total' }}
+            onPatch={(patch: Record<string, unknown>) => setFields(patch.fields as typeof fields)}
+            onClearSelection={() => {}}
+            onSelectionChange={() => {}}
+            readOnly={false}
+            locale={'en-US'}
+            onBlockingIssuesChange={report}
+          />
+        </>
+      );
+    }
+    render(<TypeSwitchHarness />);
+
+    fireEvent.change(controlFor('Formula (CEL)'), { target: { value: 'record.est_hours *' } });
+    await waitFor(() => expect(current()).toBe(1), { timeout: 3000 });
+
+    fireEvent.click(screen.getByRole('button', { name: 'retype as text' }));
+    await waitFor(() => expect(screen.queryByText('Formula (CEL)')).toBeNull(), { timeout: 3000 });
+    await waitFor(() => expect(current()).toBe(0), { timeout: 3000 });
+  });
+});