From 749f5ea019d9409c4d845958b0e28842c938fa29 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 05:52:03 +0000 Subject: [PATCH] =?UTF-8?q?fix(app-shell):=20inspectors=20can=20block=20Sa?= =?UTF-8?q?ve=20=E2=80=94=20the=20field=20inspector=20gates=20on=20CEL=20e?= =?UTF-8?q?rrors=20(#4306)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A formula that fails to parse showed its inline error and saved anyway: `record.est_hours *` left "Save draft" enabled, PUT returned 200 with a success toast, and publishing made the malformed expression the live field definition. The RLS editor refused the same input, so one console disagreed with itself. `CelPredicateField` always reported findings 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 whose Save belongs to a host, reached only through `MetadataInspectorProps` — which had no way to say "what I am showing is not saveable". So there was no channel to wire, and adding one is the fix. - `MetadataInspectorProps` gains an optional `onBlockingIssuesChange(count)`, named for blocking issues rather than CEL so the five other inspectors with the same gap (#4527) wire against this contract instead of a renamed one. - `ObjectFieldInspector` aggregates its four CEL editors into a per-site map, not one running total: four editors lint independently, so a shared counter would let fixing one of two broken rules hand back a writable Save. - Stale verdicts are settled by derivation, not repair — the map is stamped with the field it describes, and the formula site counts only while the field is a formula, so no render window exists in which Save is gated by an editor already gone. - Both hosts hold the count, disable their own Save with the message the RLS editor already shows, and stamp it with the selection so it expires on selection change and unmount. In the metadata editor all three save doors are gated — button, autosave timer and ⌘S. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- ...nspector-blocking-issues-save-gate-4306.md | 15 ++ .../views/metadata-admin/ResourceEditPage.tsx | 35 ++- .../metadata-admin/inspector-registry.ts | 15 ++ .../ObjectFieldInspector.celGate.test.tsx | 238 ++++++++++++++++++ .../inspectors/ObjectFieldInspector.tsx | 76 +++++- .../studio-design/DataPillar.celGate.test.tsx | 156 ++++++++++++ .../studio-design/StudioDesignSurface.tsx | 23 +- 7 files changed, 548 insertions(+), 10 deletions(-) create mode 100644 .changeset/inspector-blocking-issues-save-gate-4306.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.celGate.test.tsx create mode 100644 packages/app-shell/src/views/studio-design/DataPillar.celGate.test.tsx diff --git a/.changeset/inspector-blocking-issues-save-gate-4306.md b/.changeset/inspector-blocking-issues-save-gate-4306.md new file mode 100644 index 0000000000..e491ee90d5 --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index cd0a415bb9..f6f3b86092 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/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({ + ) => setFields(patch.fields as typeof fields)} + onClearSelection={() => {}} + onSelectionChange={() => {}} + readOnly={false} + locale={'en-US'} + onBlockingIssuesChange={report} + /> + + ); + } + render(); + + 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 }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx index 6a555700f7..9b2e59c06d 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx @@ -54,6 +54,7 @@ import { type FieldTypeId, } from '../previews/field-types'; import { CelPredicateField } from '../CelPredicateField'; +import type { CelLintIssue } from '../celAuthoring'; import { t, tFormat } from '../i18n'; @@ -172,12 +173,22 @@ function buildTypeOptions(locale?: string): Array<{ value: string; label: string /* ─────────────── Inspector ─────────────── */ +/** + * The CEL editors this inspector mounts, as aggregation keys. Errors are + * counted PER SITE rather than into one running total: the four editors lint + * independently and asynchronously, so a single shared counter would let + * whichever reported last overwrite the others — the multi-source aggregation + * bug that lets a still-broken rule hand back a writable Save (objectui#4306). + */ +type CelSite = 'formula' | 'visibleWhen' | 'readonlyWhen' | 'requiredWhen'; + export function ObjectFieldInspector({ selection, draft, onPatch, onClearSelection, onSelectionChange, + onBlockingIssuesChange, readOnly, locale, }: MetadataInspectorProps) { @@ -203,6 +214,63 @@ export function ObjectFieldInspector({ formulaEdited.current = false; }, [name]); + /* ─── Blocking CEL verdicts → the host's Save gate (objectui#4306) ───── + * + * Mirrors the shape PermissionAdvancedFacets uses for its per-clause map: + * identity-preserving writes (so an unchanged verdict re-renders nothing), + * a summed memo, and a reporter held in a ref so an unmemoized host callback + * cannot re-fire the effect. Declared above the `!entry` early return — + * these are hooks, and their order must not depend on the selection + * resolving to a live field. */ + const fieldType: string | null = entry + ? typeof entry.def.type === 'string' + ? (entry.def.type as string) + : 'text' + : null; + // The map is STAMPED with the field it describes, and the stale-verdict + // rules are read at aggregation time rather than repaired by reset effects. + // Deriving beats correcting here: an effect-based prune leaves a one-render + // window in which Save is still gated by an editor that is already gone. + const [celErrors, setCelErrors] = React.useState<{ + field: string; + sites: Partial>; + }>({ field: name, sites: {} }); + const reportCel = React.useCallback( + (site: CelSite, issues: CelLintIssue[]) => { + const errs = issues.filter((i) => i.severity === 'error').length; + setCelErrors((prev) => { + // A verdict that arrives after the selection moved describes the field + // now on screen, not the one it was queued for. + if (prev.field !== name) return { field: name, sites: { [site]: errs } }; + if (prev.sites[site] === errs) return prev; + return { field: name, sites: { ...prev.sites, [site]: errs } }; + }); + }, + [name], + ); + + const blockingIssues = React.useMemo(() => { + // A different field re-binds every editor: the outgoing field's verdicts + // must never gate Save for the one now open. + if (celErrors.field !== name) return 0; + let total = 0; + for (const [site, count] of Object.entries(celErrors.sites)) { + // The formula editor exists only while the field IS a formula. Counting + // a verdict it left behind would wedge Save shut with no editor on + // screen to fix it (#4306 ruling item 2). + if (site === 'formula' && fieldType !== 'formula') continue; + total += count ?? 0; + } + return total; + }, [celErrors, name, fieldType]); + const onBlockingIssuesChangeRef = React.useRef(onBlockingIssuesChange); + React.useEffect(() => { + onBlockingIssuesChangeRef.current = onBlockingIssuesChange; + }); + React.useEffect(() => { + onBlockingIssuesChangeRef.current?.(blockingIssues); + }, [blockingIssues]); + if (!entry) { return ( reportCel('formula', issues)} onInferredTypeChange={(inferred) => { if (!formulaEdited.current) return; // Spec: `returnType` carries only a PROVEN concrete type — @@ -617,6 +688,7 @@ export function ObjectFieldInspector({ label={tr('designer.field.visibleWhen')} value={readPredicate(def.visibleWhen)} onChange={(v) => patchDef({ visibleWhen: writePredicate(def.visibleWhen, v) })} + onLintChange={(issues) => reportCel('visibleWhen', issues)} disabled={readOnly} placeholder="record.status != 'draft'" objectName={typeof (draft as any).name === 'string' ? ((draft as any).name as string) : undefined} @@ -630,6 +702,7 @@ export function ObjectFieldInspector({ label={tr('designer.field.readonlyWhen')} value={readPredicate(def.readonlyWhen)} onChange={(v) => patchDef({ readonlyWhen: writePredicate(def.readonlyWhen, v) })} + onLintChange={(issues) => reportCel('readonlyWhen', issues)} disabled={readOnly} placeholder="record.status == 'closed'" objectName={typeof (draft as any).name === 'string' ? ((draft as any).name as string) : undefined} @@ -643,6 +716,7 @@ export function ObjectFieldInspector({ label={tr('designer.field.requiredWhen')} value={readPredicate(def.requiredWhen)} onChange={(v) => patchDef({ requiredWhen: writePredicate(def.requiredWhen, v) })} + onLintChange={(issues) => reportCel('requiredWhen', issues)} disabled={readOnly} placeholder="record.amount > 10000" objectName={typeof (draft as any).name === 'string' ? ((draft as any).name as string) : undefined} diff --git a/packages/app-shell/src/views/studio-design/DataPillar.celGate.test.tsx b/packages/app-shell/src/views/studio-design/DataPillar.celGate.test.tsx new file mode 100644 index 0000000000..e2262a1db0 --- /dev/null +++ b/packages/app-shell/src/views/studio-design/DataPillar.celGate.test.tsx @@ -0,0 +1,156 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * "Save draft" must refuse a formula that does not parse — objectui#4306. + * + * This is the card's literal repro, end to end in the host that owns the + * button: metadata designer, Data pillar, a formula field, `record.est_hours *` + * in the formula box. Before the fix that button stayed enabled, the PUT + * returned 200 with a success toast, and publishing made the malformed + * expression the live field definition. + * + * The inspector-side aggregation is pinned in + * {@link file://../metadata-admin/inspectors/ObjectFieldInspector.celGate.test.tsx}; + * what this suite adds is the half that only the HOST can prove: + * + * - the count actually reaches the button and disables it, with the same + * message the RLS editor already shows; + * - the host RESETS its own count when the selection changes or the inspector + * unmounts (ruling sub-decision A) — writability must never depend on a + * child's teardown running, or closing the panel on a faulty formula leaves + * Save wedged shut with no editor on screen to fix it. + * + * The engine is stubbed so the verdict is deterministic; the live lint is + * CelPredicateField.test.tsx's job. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const objectDef = { + name: 'showcase_task', + label: 'Task', + fields: [ + { name: 'est_hours', label: 'Est hours', type: 'number' }, + { name: 'total', label: 'Total', type: 'formula' }, + ], +}; + +const mockClient = { + list: vi.fn(async () => [{ name: 'showcase_task', label: 'Task' }]), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async () => ({ effective: objectDef, code: objectDef })), + getDraft: vi.fn(async () => null), +}; + +vi.mock('../metadata-admin/useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ entries: [] }), + }; +}); + +vi.mock('./packages-io', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, fetchPackages: vi.fn(async () => []) }; +}); + +vi.mock('@object-ui/react', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, useAdapter: () => ({}) }; +}); + +import { DataPillar } from './StudioDesignSurface'; +import { registerBuiltinInspectors } from '../metadata-admin/inspectors'; +import { __setCelFormulaLoader } from '../metadata-admin/celAuthoring'; + +// The right rail resolves the field inspector through the registry. +registerBuiltinInspectors(); + +afterEach(() => { + cleanup(); + __setCelFormulaLoader(undefined); +}); + +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'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'number' as const, + }), + ); +} + +const saveDraft = () => screen.getByRole('button', { name: /Save draft/i }); + +/** Open the Data pillar's form tab and select the formula field. */ +async function openFormulaField() { + stubEngine(); + render( + + + , + ); + fireEvent.click(await screen.findByRole('button', { name: 'Form' })); + const card = (await screen.findByText('Total')).closest('.cursor-grab') as HTMLElement; + fireEvent.click(card); + const label = await screen.findByText('Formula (CEL)'); + return label.parentElement!.querySelector('[role="combobox"]') as HTMLTextAreaElement; +} + +describe('DataPillar — Save draft is gated on the field inspector’s CEL verdict (#4306)', () => { + it("refuses the card's repro: a dangling operator disables Save draft", async () => { + const box = await openFormulaField(); + + // A valid formula first: this both dirties the draft (so Save is live at + // all) and pins the must-not-change half — a good formula never blocks. + fireEvent.change(box, { target: { value: 'record.est_hours * 2' } }); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 3000 }); + + // Now the card's exact input. + fireEvent.change(box, { target: { value: 'record.est_hours *' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 3000 }); + expect(saveDraft()).toHaveAttribute('title', 'Fix the CEL syntax errors before saving.'); + }); + + it('re-enables Save draft once the formula parses again', async () => { + const box = await openFormulaField(); + + fireEvent.change(box, { target: { value: 'record.est_hours *' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 3000 }); + + fireEvent.change(box, { target: { value: 'record.est_hours * 2' } }); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 3000 }); + }); + + /** + * Sub-decision A. Closing the panel unmounts the inspector, so nothing will + * ever report `0` for it — the host must drop the count on its own or Save + * stays disabled forever with no editor on screen to fix. + */ + it('drops the count when the inspector closes, so Save cannot wedge shut', async () => { + const box = await openFormulaField(); + + fireEvent.change(box, { target: { value: 'record.est_hours *' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 3000 }); + + // Two "Close" buttons dismiss the panel — the rail header's and the + // inspector shell's own; either clears the selection. + fireEvent.click(screen.getAllByRole('button', { name: 'Close' })[0]); + await waitFor(() => expect(screen.queryByText('Formula (CEL)')).toBeNull(), { timeout: 3000 }); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 3000 }); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx index f56f8b4523..5dd94610f8 100644 --- a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx @@ -1962,6 +1962,17 @@ export function DataPillar({ const [error, setError] = React.useState(null); // field management — a selected field opens ObjectFieldInspector (full type + config) const [fieldSel, setFieldSel] = React.useState(null); + // Blocking author-time issues the field inspector is showing — a CEL formula + // that does not parse must not be saveable, let alone publishable as the live + // field definition (objectui#4306). + // + // The count is STAMPED with the selection it came from, so it expires by + // construction 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 wedged shut with nothing on screen to fix. + const [blockingReport, setBlockingReport] = React.useState({ key: '', count: 0 }); + const fieldSelKey = fieldSel ? `${current?.name ?? ''}:${fieldSel.kind}:${fieldSel.id}` : ''; + const inspectorBlocking = blockingReport.key === fieldSelKey ? blockingReport.count : 0; const [dirty, setDirty] = React.useState(false); const [hasDraft, setHasDraft] = React.useState(false); const [saving, setSaving] = React.useState(false); @@ -2259,8 +2270,14 @@ export function DataPillar({ )}