diff --git a/.changeset/default-inspector-family-cel-save-gate-4527.md b/.changeset/default-inspector-family-cel-save-gate-4527.md new file mode 100644 index 000000000..906d50f7e --- /dev/null +++ b/.changeset/default-inspector-family-cel-save-gate-4527.md @@ -0,0 +1,15 @@ +--- +'@object-ui/app-shell': patch +--- + +The default-inspector family and its panel hosts gate Save on CEL errors — a hook guard, an action predicate or a validation rule that does not parse no longer saves + +#4306 gave the SCOPED inspectors a way to say "what I am showing is not saveable" (`MetadataInspectorProps.onBlockingIssuesChange`), and #4527's first half wired the shared CEL editors to it. That left the other half of the console still publishing malformed expressions, for a structural reason rather than an oversight: there are TWO inspector registries, and only one of them had the channel. `MetadataDefaultInspectorProps` — the contract every "no selection" inspector is rendered through — had no such member, so the hook guard, an action's `visible` / `disabled` predicates and a view's conditional-formatting rules on the home panel rendered their inline parse errors while Save stayed writable, and no host could pass a callback that did not exist. + +`MetadataDefaultInspectorProps` now carries the same optional `onBlockingIssuesChange`. `HookDefaultInspector` reports its guard; `ActionDefaultInspector` aggregates its two predicate editors through a per-site map, because two editors lint independently and a shared counter would hand back a writable Save the moment one of two broken predicates was fixed; the view home panel already aggregated and now has a contract to report through. + +The hosts that own the buttons hold and expire those counts. The metadata editor gates its no-selection branch as well as its scoped one, stamping each so neither reads the other's verdict. Studio's design pillar gates its rail at last — that was an unfinished edge of #4306 rather than new ground, since the same malformed-CEL publish was reachable there with the gate inert, including for the field inspector. The Data pillar gains a second count for its panel family: the validations, actions and settings panels write through the object draft and own no Save, so their faults have to reach the pillar's button, and the count is stamped with the panel tab because only one panel is mounted at a time and a tab the author has left can never retract its verdict. The hooks panel is the one panel that writes on its own (`client.save('hook', …)`), so it gates its own per-hook Save. + +Every count is DERIVED from what it describes rather than repaired by a reset effect, and pruned by what still exists: a deleted validation rule or action drops out of the total immediately, so a fault can never wedge Save shut with no editor left on screen to fix it in. A faulty rule the author merely navigates away from stays counted, because it is still in the document and saving would still publish it. + +Also wired: the object validations panel, a sixth `ConditionBuilder` consumer that the original report did not list. Still deferred by ruling: `widgets.tsx`'s condition widget, a `SchemaForm` widget needing widget-context plumbing. diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.defaultGate.test.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.defaultGate.test.tsx new file mode 100644 index 000000000..9f5e73808 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.defaultGate.test.tsx @@ -0,0 +1,153 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The metadata editor's Save must refuse a DEFAULT-inspector fault too — + * objectui#4527 phase 2. + * + * PR #4547 gated this host on the SCOPED inspector's verdict + * ({@link file://./ResourceEditPage.celGate.test.tsx}). With no selection the + * editor renders the registered DEFAULT inspector instead, and that branch + * passed no `onBlockingIssuesChange` — `MetadataDefaultInspectorProps` had no + * such member to pass. A view's conditional-formatting rules live on exactly + * that branch, so a rule whose CEL does not parse saved and published. + * + * ## Why `view`, measured rather than assumed + * + * This host only reaches a default inspector when the type ALSO has a canvas + * preview: the panel that renders it sits inside the `PreviewComponent` branch, + * and a type without one falls through to a plain `SchemaForm`. Of the types + * that have both, `view` is the one whose default inspector mounts CEL + * (`ViewDefaultInspector` -> `ViewVariantInspector` -> the formatting editor), + * so it is the type this gate is actually reachable through. The `hook` and + * `action` default inspectors are NOT reachable from this host at all — they + * are edited through the Studio panels and gated in their own suites. + * + * Only the canvas is stubbed, and only so that a preview exists and no + * selection is emitted; everything under test — the registered default + * inspector, the real formatting editor, this host's real hold and its real + * Save button — is the shipping code. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const viewDef = { + name: 'invoices', + label: 'Invoices', + list: { + type: 'grid', + object: 'invoice', + columns: ['status', 'amount'], + conditionalFormatting: [{ condition: '', style: {} }], + }, +}; + +const mockClient = { + list: vi.fn(async () => []), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async () => ({ effective: viewDef, code: viewDef, editable: true })), + getDraft: vi.fn(async () => null), + get: vi.fn(async () => null), + saveDraft: vi.fn(async () => ({})), +}; + +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ + entries: [{ type: 'view', name: 'view', label: 'View', allowOrgOverride: true }], + }), + }; +}); + +import { MetadataResourceEditPage } from './ResourceEditPage'; +import { registerBuiltinInspectors } from './inspectors'; +import { registerMetadataPreview, getMetadataPreview } from './preview-registry'; +import { __setCelFormulaLoader } from './celAuthoring'; + +registerBuiltinInspectors(); + +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: ['status', 'amount'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +/** + * Canvas stand-in: exists so the host takes its split-editor branch, and emits + * no selection, so the DEFAULT inspector is what fills the rail. + */ +function StubViewCanvas() { + return
; +} + +const realViewPreview = getMetadataPreview('view'); + +beforeEach(() => { + stubEngine(); + registerMetadataPreview('view', StubViewCanvas as never); +}); + +afterEach(() => { + cleanup(); + __setCelFormulaLoader(undefined); + if (realViewPreview) registerMetadataPreview('view', realViewPreview); +}); + +/** The Save icon button, identified by its title in either state. */ +const saveButton = () => + screen.getByRole('button', { name: /Save \(⌘S\)|Fix the CEL syntax errors before saving\./ }); + +const ruleBox = () => + screen.getByTestId('cf-rule-0').querySelector('[role="combobox"]') as HTMLTextAreaElement; + +/** Open the view editor and hand back its first formatting rule's CEL box. */ +async function openRule() { + render( + + + , + ); + await screen.findByTestId('cf-rule-0'); + return ruleBox(); +} + +describe('MetadataResourceEditPage — Save is gated on the DEFAULT inspector’s CEL verdict (#4527)', () => { + it('refuses a formatting rule whose condition does not parse', async () => { + const box = await openRule(); + + // A valid condition first: dirties the draft (so Save is live at all) and + // pins the must-not-change half — a good condition never blocks. + fireEvent.change(box, { target: { value: "record.status == 'overdue'" } }); + await waitFor(() => expect(saveButton()).toBeEnabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: 'record.amount >' } }); + await waitFor(() => expect(saveButton()).toBeDisabled(), { timeout: 4000 }); + expect(saveButton()).toHaveAttribute('title', 'Fix the CEL syntax errors before saving.'); + }); + + it('re-enables Save once the condition parses again', async () => { + const box = await openRule(); + + fireEvent.change(box, { target: { value: 'record.amount >' } }); + await waitFor(() => expect(saveButton()).toBeDisabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: "record.status == 'overdue'" } }); + await waitFor(() => expect(saveButton()).toBeEnabled(), { timeout: 4000 }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index f6f3b8609..c6891457b 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -508,7 +508,14 @@ function MetadataResourceEditPageImpl({ // 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}` : ''; + // Covers BOTH inspector branches. With no selection the editor renders the + // registered DEFAULT inspector instead, and that surface hosts CEL too (a + // hook's guard, an action's predicates, a view's formatting rules) — so it + // gets its own stamp rather than sharing the scoped one (objectui#4527). + // Distinct keys are what stop one branch's verdict from gating the other. + const selectionKey = selection + ? `${type}:${name}:${selection.kind}:${selection.id}` + : `${type}:${name}:default`; const inspectorBlocking = blockingReport.key === selectionKey ? blockingReport.count : 0; React.useEffect(() => { if (!editing) setSelection(null); @@ -2349,6 +2356,9 @@ function MetadataResourceEditPageImpl({ })) } onSelectionChange={setSelection} + onBlockingIssuesChange={(count) => + setBlockingReport({ key: selectionKey, count }) + } readOnly={formReadOnly} locale={locale} serverSchema={entry?.schema as Record | undefined} diff --git a/packages/app-shell/src/views/metadata-admin/default-inspector-registry.ts b/packages/app-shell/src/views/metadata-admin/default-inspector-registry.ts index bd56d9c68..53caf1129 100644 --- a/packages/app-shell/src/views/metadata-admin/default-inspector-registry.ts +++ b/packages/app-shell/src/views/metadata-admin/default-inspector-registry.ts @@ -33,6 +33,26 @@ export interface MetadataDefaultInspectorProps { * scoped inspector for that selection. */ 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#4527). + * + * Symmetric with `MetadataInspectorProps.onBlockingIssuesChange` (#4306), and + * deliberately the SAME shape: the default (no-selection) inspectors host CEL + * editors too — the hook guard, an action's visible/disabled predicates, a + * view's conditional formatting — and the host owns Save, so only the host + * can refuse to write. Without this member no host could pass the callback at + * all, which is why the whole default-inspector family published malformed + * expressions while the scoped family already refused them. + * + * Fires whenever the aggregate changes, `0` when everything is clean. + * + * Optional — an inspector with nothing to block on simply never calls it. + * Hosts must expire their own count when the inspected item 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. */ diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.celGate.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.celGate.test.tsx new file mode 100644 index 000000000..efe9dfee4 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.celGate.test.tsx @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The Action inspector must REPORT its blocking CEL verdicts upward, so the + * host that owns Save can refuse to publish a predicate that does not parse — + * objectui#4527 phase 2. + * + * This inspector mounts TWO ConditionBuilders — "Visible when" (`visible`) and + * "Disabled when" (`disabled`) — and is a DEFAULT inspector, so before phase 2 + * it had no channel to report through at all. + * + * ## The decisive case + * + * Two editors report independently and asynchronously, so a single shared + * counter lets whichever linted last overwrite the other: fixing "Visible when" + * while "Disabled when" is still malformed would hand back a writable Save. + * A shared counter passes every single-editor case below and fails only + * {@link https://github.com/objectstack-ai/objectui/issues/4527 the both-faulty + * case} — which is why the count is a per-SITE map, exactly as + * ObjectFieldInspector's is. + */ + +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, within } from '@testing-library/react'; + +import { ActionDefaultInspector } from './ActionDefaultInspector'; +import { __setCelFormulaLoader } from '../celAuthoring'; + +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: ['status'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +/** + * Stateful harness — the inspector is CONTROLLED, so committed predicates must + * round-trip through the draft or the next keystroke reverts the last one and + * the editors never lint what the author typed. + */ +function Harness({ report }: { report: (n: number) => void }) { + const [draft, setDraft] = React.useState>({ + name: 'approve', + label: 'Approve', + type: 'script', + }); + return ( + setDraft((d) => ({ ...d, ...patch }))} + readOnly={false} + locale={'en-US' as never} + onBlockingIssuesChange={report} + /> + ); +} + +function renderInspector() { + const report = vi.fn(); + render(); + const current = () => report.mock.calls.at(-1)?.[0] as number | undefined; + return { report, current }; +} + +/** + * A ConditionBuilder's own root, located by its label — the two builders are + * otherwise identical, so every interaction must be scoped to one of them. + */ +function builder(label: string): HTMLElement { + return screen.getByText(label).parentElement!.parentElement! as HTMLElement; +} + +/** Switch one builder into raw CEL mode and hand back its editor. */ +function rawEditorFor(label: string): HTMLTextAreaElement { + const root = builder(label); + fireEvent.click(within(root).getByText('Expression')); + return within(root) + .getAllByRole('combobox') + .find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; +} + +describe('ActionDefaultInspector — blocking CEL issues reach the host (#4527)', () => { + it('counts a "Visible when" predicate that does not parse', async () => { + stubEngine(); + const { current } = renderInspector(); + fireEvent.change(rawEditorFor('Visible when'), { target: { value: 'record.status ==' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + }); + + it('counts a "Disabled when" predicate that does not parse', async () => { + stubEngine(); + const { current } = renderInspector(); + fireEvent.change(rawEditorFor('Disabled when'), { target: { value: 'record.amount >' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + }); + + it('reports clean predicates as zero, so valid conditions never block Save', async () => { + stubEngine(); + const { current } = renderInspector(); + fireEvent.change(rawEditorFor('Visible when'), { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(current()).toBe(0), { timeout: 3000 }); + }); + + /** + * DECISIVE — the per-site map. One shared counter passes every case above + * and fails here: fixing one editor would drop the total to 0 while the + * other predicate is still malformed. + */ + it('keeps each editor independent — fixing one leaves the other counted', async () => { + stubEngine(); + const { current } = renderInspector(); + const visible = rawEditorFor('Visible when'); + const disabled = rawEditorFor('Disabled when'); + + fireEvent.change(visible, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + + fireEvent.change(disabled, { target: { value: 'record.amount >' } }); + await waitFor(() => expect(current()).toBe(2), { timeout: 3000 }); + + fireEvent.change(visible, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx index 319b06c2a..edb67c07e 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx @@ -276,12 +276,23 @@ interface ActionParam { /* ─────────────── inspector ─────────────── */ +/** + * The CEL editors this inspector mounts, as aggregation keys. Errors are + * counted PER SITE rather than into one running total: the two predicate + * editors lint independently and asynchronously, so a single shared counter + * would let whichever reported last overwrite the other — fixing "Visible + * when" would hand back a writable Save while "Disabled when" was still + * malformed (objectui#4527, the shape ObjectFieldInspector uses for its four). + */ +type ActionCelSite = 'visible' | 'disabled'; + export function ActionDefaultInspector({ draft, onPatch, readOnly, locale, serverSchema, + onBlockingIssuesChange, }: MetadataDefaultInspectorProps) { const tr = React.useCallback((key: string) => t(key, locale), [locale]); @@ -296,6 +307,42 @@ export function ActionDefaultInspector({ const aiExposed = draft.aiExposed === true; const aiDescription = typeof draft.aiDescription === 'string' ? (draft.aiDescription as string) : ''; + /* ─── Blocking CEL verdicts → the host's Save gate (objectui#4527) ───── + * + * Per-site map (see {@link ActionCelSite}), STAMPED with the action it + * describes so a verdict that lands after the panel switched actions cannot + * gate the one now on screen. Mismatch is read as 0 at aggregation time + * rather than repaired by a reset effect. */ + const actionKey = str('name'); + const [celErrors, setCelErrors] = React.useState<{ + action: string; + sites: Partial>; + }>({ action: actionKey, sites: {} }); + const reportCel = React.useCallback( + (site: ActionCelSite, count: number) => { + setCelErrors((prev) => { + if (prev.action !== actionKey) return { action: actionKey, sites: { [site]: count } }; + if (prev.sites[site] === count) return prev; + return { action: actionKey, sites: { ...prev.sites, [site]: count } }; + }); + }, + [actionKey], + ); + const blockingIssues = React.useMemo(() => { + if (celErrors.action !== actionKey) return 0; + let total = 0; + for (const count of Object.values(celErrors.sites)) total += count ?? 0; + return total; + }, [celErrors, actionKey]); + // Held in a ref so an unmemoized host callback cannot re-fire the effect. + const onBlockingIssuesChangeRef = React.useRef(onBlockingIssuesChange); + React.useEffect(() => { + onBlockingIssuesChangeRef.current = onBlockingIssuesChange; + }); + React.useEffect(() => { + onBlockingIssuesChangeRef.current?.(blockingIssues); + }, [blockingIssues]); + const patchBody = (p: Record) => onPatch({ body: { ...body, ...p } }); const patchParam = (i: number, p: Partial) => onPatch({ params: params.map((it, j) => (j === i ? { ...it, ...p } : it)) }); @@ -474,8 +521,8 @@ export function ActionDefaultInspector({ {/* Both are `ExpressionInputSchema` in the spec (`disabled` as `boolean | ExpressionInput`), so a persisted action carries the ADR-0089 envelope — same read/write pair as the hook guard (#3218). */} - onPatch({ visible: writeExpressionSource(draft.visible, v) })} objectName={objectName} disabled={readOnly} /> - onPatch({ disabled: writeExpressionSource(draft.disabled, v) })} objectName={objectName} disabled={readOnly} /> + onPatch({ visible: writeExpressionSource(draft.visible, v) })} objectName={objectName} disabled={readOnly} onBlockingIssuesChange={(n) => reportCel('visible', n)} /> + onPatch({ disabled: writeExpressionSource(draft.disabled, v) })} objectName={objectName} disabled={readOnly} onBlockingIssuesChange={(n) => reportCel('disabled', n)} />
{/* 7 ─ AI exposure */} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.celGate.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.celGate.test.tsx new file mode 100644 index 000000000..e95afc6b7 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.celGate.test.tsx @@ -0,0 +1,117 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The Hook inspector must REPORT its blocking CEL verdict upward, so the host + * that owns Save can refuse to publish a guard that does not parse — + * objectui#4527 phase 2. + * + * Phase 1 (PR #4547) wired the SCOPED inspector family through + * `MetadataInspectorProps.onBlockingIssuesChange`. This inspector is a DEFAULT + * inspector: it takes `MetadataDefaultInspectorProps`, whose contract carried + * no such member, so its "Run only when" guard rendered its inline parse error + * and Save still saved. Phase 2 extends that contract symmetrically. + * + * The engine is stubbed deterministically — the live lint is + * `CelPredicateField.test.tsx`'s job; this suite tests WIRING. + */ + +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'; +import { HookSchema } from '@objectstack/spec/data'; + +import { HookDefaultInspector } from './HookDefaultInspector'; +import { __setCelFormulaLoader } from '../celAuthoring'; + +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: ['status'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +/** + * Author a hook the way a user does and parse it with the spec. `object: '*'` + * keeps ConditionBuilder in raw/no-catalog mode, so no field fetch is involved + * — the reporting channel is what is under test. + */ +function hookDraft(condition?: unknown): Record { + return HookSchema.parse({ + name: 'guard_hook', + object: '*', + events: ['beforeInsert'], + handler: 'guard_fn', + ...(condition === undefined ? {} : { condition }), + }) as unknown as Record; +} + +/** + * Stateful harness — the inspector is CONTROLLED, so the committed guard must + * round-trip through the draft or the next keystroke reverts the last one and + * the editor never lints what the author typed. + */ +function Harness({ initial, report }: { initial: Record; report: (n: number) => void }) { + const [draft, setDraft] = React.useState(initial); + return ( + setDraft((d) => ({ ...d, ...patch }))} + readOnly={false} + locale={'en-US' as never} + onBlockingIssuesChange={report} + /> + ); +} + +/** Render and switch the guard into its raw CEL editor. */ +function renderRaw(condition?: unknown) { + const report = vi.fn(); + render(); + fireEvent.click(screen.getByText('Expression')); + const box = screen.getAllByRole('combobox').find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; + const current = () => report.mock.calls.at(-1)?.[0] as number | undefined; + return { report, current, box }; +} + +describe('HookDefaultInspector — blocking CEL issues reach the host (#4527)', () => { + it('counts a "Run only when" guard that does not parse', async () => { + stubEngine(); + const { current, box } = renderRaw(); + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + }); + + it('reports a clean guard as zero, so a valid condition never blocks Save', async () => { + stubEngine(); + const { current, box } = renderRaw(); + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(current()).toBe(0), { timeout: 3000 }); + }); + + it('re-enables Save when the author fixes the guard', async () => { + stubEngine(); + const { current, box } = renderRaw(); + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(current()).toBe(0), { timeout: 3000 }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.tsx index 8b33505bb..d4845a319 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/HookDefaultInspector.tsx @@ -115,6 +115,7 @@ export function HookDefaultInspector({ readOnly, locale, serverSchema, + onBlockingIssuesChange, }: MetadataDefaultInspectorProps) { const tr = React.useCallback((key: string) => t(key, locale), [locale]); @@ -145,6 +146,37 @@ export function HookDefaultInspector({ onPatch({ events: next }); }; + /* ─── Blocking CEL verdicts → the host's Save gate (objectui#4527) ───── + * + * The "Run only when" guard is this inspector's only CEL site. The count is + * STAMPED with the hook it describes and mismatch is read as 0 at + * aggregation time, so a verdict that lands after the host switched hooks + * cannot gate the one now on screen — derivation, not a reset effect. */ + const hookKey = str('name'); + const [celErrors, setCelErrors] = React.useState<{ hook: string; count: number }>({ + hook: hookKey, + count: 0, + }); + const reportCel = React.useCallback( + (count: number) => { + setCelErrors((prev) => { + if (prev.hook !== hookKey) return { hook: hookKey, count }; + if (prev.count === count) return prev; + return { hook: hookKey, count }; + }); + }, + [hookKey], + ); + const blockingIssues = celErrors.hook === hookKey ? celErrors.count : 0; + // Held in a ref so an unmemoized host callback cannot re-fire the effect. + const onBlockingIssuesChangeRef = React.useRef(onBlockingIssuesChange); + React.useEffect(() => { + onBlockingIssuesChangeRef.current = onBlockingIssuesChange; + }); + React.useEffect(() => { + onBlockingIssuesChangeRef.current?.(blockingIssues); + }, [blockingIssues]); + // A single object → give ConditionBuilder its fields; '*' / multi → raw mode. const conditionObject = !allObjects && objectNames.length === 1 ? objectNames[0] : undefined; const language = typeof body.language === 'string' ? (body.language as string) : 'expression'; @@ -262,6 +294,7 @@ export function HookDefaultInspector({ onCommit={(v) => onPatch({ condition: writeExpressionSource(draft.condition, v) })} objectName={conditionObject} disabled={readOnly} + onBlockingIssuesChange={reportCel} /> diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ViewVariantInspector.homeGate.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ViewVariantInspector.homeGate.test.tsx new file mode 100644 index 000000000..1354c3f78 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ViewVariantInspector.homeGate.test.tsx @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The View "home" panel must report its blocking CEL verdicts too — + * objectui#4527 phase 2. + * + * Phase 1 (PR #4547) wired `ViewVariantInspector`'s aggregation and forwarded + * the channel through `ViewInspector`, the SCOPED router. The HOME path goes + * through `ViewDefaultInspector` instead, a `MetadataDefaultInspectorProps` + * component whose contract had no blocking-issues member — so the very same + * inspector, reached with no selection, reported nothing and Save stayed + * writable on a formatting rule that does not parse. + * + * That asymmetry is the whole point of this suite: the aggregation is already + * proven by + * {@link file://./ViewVariantInspector.celGate.test.tsx}; what is pinned here + * is that the HOME route carries the channel, so the fix cannot be "wired on + * one of the two paths a user can reach the same editor by". + */ + +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'; + +import { ViewDefaultInspector } from './ViewInspector'; +import type { MetadataDefaultInspectorProps } from '../default-inspector-registry'; +import { __setCelFormulaLoader } from '../celAuthoring'; + +/** + * TYPE-LEVEL pin, and the only thing on this path that was actually broken. + * + * `ViewDefaultInspector` spreads `{...props}` into `ViewVariantInspector`, so + * once phase 1 taught the variant inspector to aggregate, the callback ALREADY + * reached it at runtime — the two render cases below pass before phase 2 as + * well as after, and are pins rather than red signals (reported as such). + * + * What was missing is the CONTRACT: `MetadataDefaultInspectorProps` did not + * declare the member, so no host could pass it without a type error and none + * did. This assertion is the red one — it fails to compile in the + * `tsconfig.test.json` pass until the contract carries the channel. + */ +export const _contractCarriesBlockingChannel: Required< + Pick +> = { onBlockingIssuesChange: () => {} }; + +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: ['status', 'amount'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +function viewDraft(rules: unknown[]): Record { + return { + name: 'invoices', + label: 'Invoices', + list: { + type: 'grid', + object: 'invoice', + columns: ['status', 'amount'], + conditionalFormatting: rules, + }, + }; +} + +/** Controlled harness — patches must round-trip or the second edit reverts. */ +function Harness({ report }: { report: (n: number) => void }) { + const [draft, setDraft] = React.useState(viewDraft([{ condition: '', style: {} }])); + return ( + setDraft((d) => ({ ...d, ...patch }))} + onSelectionChange={() => {}} + readOnly={false} + locale={'en-US' as never} + onBlockingIssuesChange={report} + /> + ); +} + +const ruleBox = (i: number) => + screen.getByTestId(`cf-rule-${i}`).querySelector('[role="combobox"]') as HTMLTextAreaElement; + +describe('ViewDefaultInspector — the home panel reports blocking CEL issues too (#4527)', () => { + it('counts a formatting condition that does not parse on the HOME path', async () => { + stubEngine(); + const report = vi.fn(); + render(); + const current = () => report.mock.calls.at(-1)?.[0] as number | undefined; + + fireEvent.change(ruleBox(0), { target: { value: 'record.amount >' } }); + await waitFor(() => expect(current()).toBe(1), { timeout: 3000 }); + }); + + it('reports a clean rule as zero on the HOME path', async () => { + stubEngine(); + const report = vi.fn(); + render(); + const current = () => report.mock.calls.at(-1)?.[0] as number | undefined; + + fireEvent.change(ruleBox(0), { target: { value: "record.status == 'overdue'" } }); + await waitFor(() => expect(current()).toBe(0), { timeout: 3000 }); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/DataPillar.panelGate.test.tsx b/packages/app-shell/src/views/studio-design/DataPillar.panelGate.test.tsx new file mode 100644 index 000000000..f8fa72fb5 --- /dev/null +++ b/packages/app-shell/src/views/studio-design/DataPillar.panelGate.test.tsx @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The Data pillar's "Save draft" must refuse an object whose VALIDATION RULE + * guard does not parse — objectui#4527 phase 2. + * + * #4536 gated this same button on the field inspector's CEL verdict + * ({@link file://./DataPillar.celGate.test.tsx}). The pillar hosts three more + * CEL-bearing surfaces that write through the very same draft and were left + * ungated: the validations panel (rule guards), the actions panel (an action's + * visible/disabled predicates) and the settings panel. None of them owns a Save + * — their own headers say the Data pillar's Save draft owns the write — so the + * pillar holds a SECOND stamped count for the panel family and folds it into + * the same button. + * + * The stamp is the panel TAB: only one panel is mounted at a time, so leaving + * the tab unmounts the reporter and it can never retract its last verdict. + * Deriving the count against the live tab is what stops a fault authored under + * "Rules" from wedging Save shut while the author is looking at "Fields". + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, afterEach, beforeEach } 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: 'status', label: 'Status', type: 'text' }], + validations: [ + { + type: 'script', + name: 'rule_a', + label: 'Rule A', + message: 'nope', + severity: 'error', + active: true, + }, + ], +}; + +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'; + +registerBuiltinInspectors(); + +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: ['status'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +beforeEach(stubEngine); +afterEach(() => { + cleanup(); + __setCelFormulaLoader(undefined); +}); + +const saveDraft = () => screen.getByRole('button', { name: /Save draft/i }); + +/** Open the pillar's Rules tab and put the selected rule's guard into raw CEL. */ +async function openRuleGuard() { + render( + + + , + ); + fireEvent.click(await screen.findByRole('button', { name: 'Validations' })); + fireEvent.click(await screen.findByText('Expression')); + return screen.getAllByRole('combobox').find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; +} + +describe('DataPillar — Save draft is gated on the validations panel’s CEL verdict (#4527)', () => { + it('refuses a rule guard that does not parse', async () => { + const box = await openRuleGuard(); + + // A valid guard first — dirties the draft and pins that a good guard never + // blocks. + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 4000 }); + expect(saveDraft()).toHaveAttribute('title', 'Fix the CEL syntax errors before saving.'); + }); + + it('re-enables Save draft once the guard parses again', async () => { + const box = await openRuleGuard(); + + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 4000 }); + }); + + /** + * The tab stamp. Leaving Rules unmounts the panel, so it can never report + * `0`; without deriving the count against the live tab, Save would stay + * wedged shut on a surface with no CEL editor on screen at all. + */ + it('expires the panel count when the author leaves the Rules tab', async () => { + const box = await openRuleGuard(); + + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveDraft()).toBeDisabled(), { timeout: 4000 }); + + fireEvent.click(screen.getByRole('button', { name: 'Form' })); + await waitFor(() => expect(saveDraft()).toBeEnabled(), { timeout: 4000 }); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/ObjectActionsPanel.tsx b/packages/app-shell/src/views/studio-design/ObjectActionsPanel.tsx index 1c4655dba..9e0524d9d 100644 --- a/packages/app-shell/src/views/studio-design/ObjectActionsPanel.tsx +++ b/packages/app-shell/src/views/studio-design/ObjectActionsPanel.tsx @@ -70,10 +70,18 @@ export function ObjectActionsPanel({ onPatch, disabled, actionSchema, + onBlockingIssuesChange, }: { draft: Record; onPatch: (patch: Record) => void; disabled?: boolean; + /** + * Report how many BLOCKING author-time issues the selected action's editor is + * showing — `visible` / `disabled` predicates whose CEL does not parse + * (objectui#4527). This panel owns no Save; the object draft's "Save draft" + * writes the actions, so the Data pillar holds this count and refuses. + */ + onBlockingIssuesChange?: (count: number) => void; /** * The live server JSONSchema for the `action` type (`/meta/types`). Handed to * ActionDefaultInspector as `serverSchema` so its "More fields" section can @@ -107,6 +115,39 @@ export function ObjectActionsPanel({ const objectName = typeof draft.name === 'string' ? draft.name : ''; + /* ─── Blocking CEL verdicts → the Data pillar's Save gate (objectui#4527) ── + * + * Master-detail: only the SELECTED action's inspector is mounted, so the map + * is keyed by action NAME and a verdict deliberately survives deselection — + * an action whose predicate does not parse is still in the document and + * saving would still publish it, and it stays reachable by selecting it + * again. A DELETED action's inspector can never report `0`, so the total is + * DERIVED against the live action-name set rather than repaired by an effect. + */ + const [celErrors, setCelErrors] = React.useState>({}); + const reportCel = React.useCallback((actionName: string, count: number) => { + setCelErrors((prev) => (prev[actionName] === count ? prev : { ...prev, [actionName]: count })); + }, []); + const liveActionNames = React.useMemo( + () => new Set(actions.map((a) => String(a.name ?? ''))), + [actions], + ); + const blockingIssues = React.useMemo(() => { + let total = 0; + for (const [name, count] of Object.entries(celErrors)) { + if (!liveActionNames.has(name)) continue; // pruned: the action is gone + total += count; + } + return total; + }, [celErrors, liveActionNames]); + const onBlockingIssuesChangeRef = React.useRef(onBlockingIssuesChange); + React.useEffect(() => { + onBlockingIssuesChangeRef.current = onBlockingIssuesChange; + }); + React.useEffect(() => { + onBlockingIssuesChangeRef.current?.(blockingIssues); + }, [blockingIssues]); + const addAction = React.useCallback(() => { const name = nextActionName(objectName, actions.map((a) => String(a.name ?? ''))); // Minimal *valid* skeleton: a script action bound to this object, seeded @@ -221,6 +262,7 @@ export function ObjectActionsPanel({ readOnly={!!disabled} locale={locale} serverSchema={actionSchema} + onBlockingIssuesChange={(count) => reportCel(String(sel.name ?? ''), count)} /> ) : ( diff --git a/packages/app-shell/src/views/studio-design/ObjectHooksPanel.celGate.test.tsx b/packages/app-shell/src/views/studio-design/ObjectHooksPanel.celGate.test.tsx new file mode 100644 index 000000000..4babfb3bf --- /dev/null +++ b/packages/app-shell/src/views/studio-design/ObjectHooksPanel.celGate.test.tsx @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The object hooks panel owns its OWN per-hook Save (it writes the hook + * directly with `client.save('hook', …, { mode: 'draft' })` — the object's Save + * draft does not cover hooks), so it must refuse to write a guard whose CEL + * does not parse — objectui#4527 phase 2. + * + * Of the four panel hosts the #4547 census surfaced, this is the only one that + * owns a Save button; the other three write through the Data pillar's draft and + * report upward instead. So this suite is the end-to-end proof for the + * hook branch of the default-inspector family: HookDefaultInspector's verdict + * has to reach a real disabled button, not just a callback. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react'; + +const hook = { + name: 'guard_hook', + label: 'Guard', + object: 'invoice', + events: ['beforeInsert'], + handler: 'guard_fn', +}; + +const mockClient = { + list: vi.fn(async () => [hook]), + listDrafts: vi.fn(async () => []), + getDraft: vi.fn(async () => null), + // The curated hook editor resolves the bound object's field catalog through + // `useObjectFields` -> `client.get`. + get: vi.fn(async () => null), + save: vi.fn(async () => ({})), +}; + +vi.mock('../metadata-admin/useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, useMetadataClient: () => mockClient }; +}); + +import { ObjectHooksPanel } from './ObjectHooksPanel'; +import { registerBuiltinInspectors } from '../metadata-admin/inspectors'; +import { __setCelFormulaLoader } from '../metadata-admin/celAuthoring'; + +// The panel resolves the curated hook editor through the default registry. +registerBuiltinInspectors(); + +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: ['status'], roots: ['record'], functions: ['has'] }), + inferExpressionType: () => 'boolean' as const, + }), + ); +} + +beforeEach(stubEngine); +afterEach(() => { + cleanup(); + __setCelFormulaLoader(undefined); +}); + +const saveButton = () => screen.getByRole('button', { name: /Save/i }); + +/** Open the panel, select the hook, and switch its guard into raw CEL mode. */ +async function openGuard() { + render(); + fireEvent.click(await screen.findByText('Guard')); + fireEvent.click(await screen.findByText('Expression')); + return screen.getAllByRole('combobox').find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; +} + +describe('ObjectHooksPanel — its own Save refuses a guard that does not parse (#4527)', () => { + it('disables Save while the hook guard is malformed', async () => { + const box = await openGuard(); + + // A valid guard first: dirties the draft (so Save is live at all) and pins + // the must-not-change half — a good guard never blocks. + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(saveButton()).toBeEnabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveButton()).toBeDisabled(), { timeout: 4000 }); + }); + + it('re-enables Save once the guard parses again', async () => { + const box = await openGuard(); + + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveButton()).toBeDisabled(), { timeout: 4000 }); + + fireEvent.change(box, { target: { value: "record.status == 'open'" } }); + await waitFor(() => expect(saveButton()).toBeEnabled(), { timeout: 4000 }); + }); + + it('never writes the malformed hook', async () => { + const box = await openGuard(); + fireEvent.change(box, { target: { value: 'record.status ==' } }); + await waitFor(() => expect(saveButton()).toBeDisabled(), { timeout: 4000 }); + + fireEvent.click(saveButton()); + expect(mockClient.save).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/ObjectHooksPanel.tsx b/packages/app-shell/src/views/studio-design/ObjectHooksPanel.tsx index 25d67febf..b720744f1 100644 --- a/packages/app-shell/src/views/studio-design/ObjectHooksPanel.tsx +++ b/packages/app-shell/src/views/studio-design/ObjectHooksPanel.tsx @@ -95,6 +95,31 @@ export function ObjectHooksPanel({ const [saving, setSaving] = React.useState(false); const [nonce, setNonce] = React.useState(0); + /* ─── Blocking CEL verdicts → this panel's OWN Save (objectui#4527) ──────── + * + * Unlike the object's other panels, this one writes the hook itself + * (`client.save('hook', …, { mode: 'draft' })`), so it owns the button that + * has to refuse. The count is STAMPED with the hook it describes and + * mismatch is read as 0, so a verdict that lands after the author switched + * hooks cannot gate the one now on screen — derivation, not a reset effect, + * and the selected hook always has its editor available to fix in. */ + const [celErrors, setCelErrors] = React.useState<{ hook: string; count: number }>({ + hook: '', + count: 0, + }); + const selectedHookName = String(draft?.name ?? ''); + const reportCel = React.useCallback( + (count: number) => { + setCelErrors((prev) => { + if (prev.hook !== selectedHookName) return { hook: selectedHookName, count }; + if (prev.count === count) return prev; + return { hook: selectedHookName, count }; + }); + }, + [selectedHookName], + ); + const blockingIssues = celErrors.hook === selectedHookName ? celErrors.count : 0; + React.useEffect(() => { let cancelled = false; setLoading(true); @@ -250,7 +275,8 @@ export function ObjectHooksPanel({