diff --git a/.changeset/6293-conditionbuilder-reference-value.md b/.changeset/6293-conditionbuilder-reference-value.md new file mode 100644 index 0000000000..1bee446a33 --- /dev/null +++ b/.changeset/6293-conditionbuilder-reference-value.md @@ -0,0 +1,39 @@ +--- +'@object-ui/app-shell': patch +--- + +`ConditionBuilder`'s row mode now compiles a value that is plainly a **reference** as one, +instead of quoting it into a string literal (objectui#6293). + +`fmtValue` quoted anything that was not a number / `true` / `false` / `null`, and the value +box is free text — so an author building "this field differs from its prior value", the idiom +that *defines* a change-detection predicate, got `previous == 'previous.status'`. That is +syntactically valid CEL, `previous` is a declared root, and a string literal's contents are +deliberately not scanned for references by `flow-ref-check` or by the server-side validator. +The predicate parsed, registered, evaluated — and was always false, with no author-time signal +at any layer, at all five surfaces that mount this builder. + +A value matching a **declared root prefix** now emits as the reference: `record`, `previous`, +`parent`, `user`, `current_user`, `org`. That set is this builder's own vocabulary — `record` +/ `user` / `org` are exactly what its subject dropdown offers one control to the left, +`previous` and `parent` are bound by `evalFieldPredicate` and by the server-side hook / +validation evaluators, and `current_user` is the ADR-0068 spelling of the same identity object +`user` names. Roots this builder never offers (`data`, `os`, `app`, `features`, `input`, +`vars`, `page`) are deliberately excluded: `data.csv` is a plausible literal and `data` *is* +bound, so capturing it would trade one silently-false predicate for another rather than for a +loud one. Declaring which roots a mounting surface actually binds is caller-supplied +vocabulary and belongs to objectui#6296. + +The test is "a dotted path under a declared root", not "contains a dot" — a version string +(`1.2.3`), a filename, and a path under an unbound root all stay literal text. The literal and +number controls are unchanged: `done` still compiles to `'done'`, `42` still to `42`. + +**Nothing already stored is rewritten.** A persisted `previous == 'previous.status'` no longer +round-trips byte-for-byte, so the builder's existing safety rule hands it to the raw CEL editor +rather than reinterpreting it — the author sees both readings and decides. In the other +direction a hand-authored `record.status != previous.status` now round-trips *into* the row +builder, which it could not before. + +The repair is at the authoring surface, where the ambiguity is: no consumer-side tolerance is +added, and the emitted reference is now an identifier the existing reference checkers can see, +where a string literal's contents were invisible to them. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.referenceValue.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.referenceValue.test.tsx new file mode 100644 index 0000000000..ba45b2d803 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.referenceValue.test.tsx @@ -0,0 +1,124 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ConditionBuilder row mode — a value that is plainly a REFERENCE compiles as + * one, instead of being quoted into a string literal (objectui#6293). + * + * The observable pinned here is the emitted CEL, not the UI state: `fmtValue` + * quoted anything that was not a number / `true` / `false` / `null`, so + * "this field differs from its prior value" — the idiom that DEFINES a + * change-detection predicate — compiled to `previous == 'previous.status'`. + * That parses, registers and evaluates, and is always false; nothing at any + * layer objects, because `previous` is a declared root and a string literal's + * contents are deliberately not scanned for references. + * + * Mounted through the real component and read off `onCommit`, because the + * commit path above `fmtValue` is part of the defect — a unit test of the + * formatter alone would not have caught a builder that never reaches it. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; + +// objectui#4697 — ConditionBuilder calls useObjectFields(objectName) +// unconditionally even when a `fields` prop is supplied, so stub the shared +// client to keep the mount-time fetch off the network. +const state = vi.hoisted(() => ({ + metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) }, +})); +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => state.metadataClient, +})); + +import { ConditionBuilder } from './ConditionBuilder'; + +afterEach(cleanup); + +const FIELDS = [ + { name: 'status', label: 'Status' }, + { name: 'done', label: 'Done' }, +]; + +/** + * Controlled harness — the hook-condition surface, opened on `previous == null` + * exactly as the card measured it (that round-trips, so the builder opens in + * ROW mode with a live value box). + */ +function Harness({ initial }: { initial: string }) { + const [v, setV] = React.useState(initial); + return ( +
+ + 
{v}
+
+ ); +} + +/** Type into the row's live value box and return the CEL that was emitted. */ +function typeIntoValueBox(text: string): string { + const input = screen.getByPlaceholderText('value') as HTMLInputElement; + fireEvent.change(input, { target: { value: text } }); + return screen.getByTestId('committed').textContent ?? ''; +} + +describe('ConditionBuilder row mode — reference vs. literal in the value box (#6293)', () => { + it('opens `previous == null` in ROW mode with a live value box (positive probe)', () => { + const { container } = render(); + expect(container.querySelector('textarea')).toBeNull(); // not the raw CEL editor + expect((screen.getByPlaceholderText('value') as HTMLInputElement).value).toBe('null'); + }); + + it('compiles a value under a declared root as the REFERENCE it plainly is', () => { + render(); + expect(typeIntoValueBox('previous.status')).toBe('previous == previous.status'); + }); + + it('compiles `record.` on the value side as a reference too', () => { + render(); + expect(typeIntoValueBox('record.status')).toBe('previous == record.status'); + }); + + it('CONTROL — a literal is still quoted', () => { + render(); + expect(typeIntoValueBox('done')).toBe("previous == 'done'"); + }); + + it('CONTROL — a number is still emitted unquoted', () => { + render(); + expect(typeIntoValueBox('42')).toBe('previous == 42'); + }); + + it('CONTROL — a dotted value under an UNDECLARED root is still quoted', () => { + // The repair keys on the declared root vocabulary, not on "contains a dot": + // `foo` is not a root any mounting surface binds, and `1.2.3` is a version + // string. Both stay literal text. + render(); + expect(typeIntoValueBox('foo.bar')).toBe("previous == 'foo.bar'"); + expect(typeIntoValueBox('1.2.3')).toBe("previous == '1.2.3'"); + }); + + it('the emitted reference round-trips — reopening it stays in ROW mode', () => { + // If it did not round-trip, the builder would flip to the raw CEL editor + // the moment the author reopened the record they had just authored. + const { container } = render(); + expect(container.querySelector('textarea')).toBeNull(); + expect((screen.getByPlaceholderText('value') as HTMLInputElement).value).toBe('previous.status'); + }); + + it('is NOT retroactive — an already-stored quoted literal opens in RAW mode, never silently rewritten', () => { + // A predicate already persisted as text (whether the author meant the text + // or hit this defect) is not the builder's to reinterpret: it no longer + // round-trips, so the component's existing safety rule hands it to the raw + // CEL editor where the author can see both readings and decide. + const { container } = render(); + expect(container.querySelector('textarea')).not.toBeNull(); + expect(screen.getByTestId('committed').textContent).toBe("previous == 'previous.status'"); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx index c69f8deda0..589f790736 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx @@ -52,15 +52,72 @@ const CONTEXT_SUBJECTS = [ const norm = (s: string) => s.replace(/\s+/g, ' ').trim(); -/** Quote a raw value for CEL unless it is a number / boolean / null. */ +/** + * Scope roots a value typed into the value box may plainly REFERENCE, rather + * than name as literal text (objectui#6293). + * + * Deliberately the roots this builder's own vocabulary already commits to — + * not every root the CEL engine advertises: + * + * - `record` / `user` / `org` — this component's own subject vocabulary + * (`record.` from the field catalog, plus {@link CONTEXT_SUBJECTS}). + * A value under one of these is the same identifier the subject dropdown + * emits one control to its left. + * - `previous` — the prior persisted record, bound by `evalFieldPredicate` + * (`@object-ui/core`) and by the server-side hook / validation evaluators. + * This is the change-detection idiom the defect was measured on. + * - `current_user` — the canonical spelling of `user` (ADR-0068); the shell + * binds one identity object under both names, so which alias the author + * happened to type must not decide whether it reads as a reference. + * - `parent` — the header row an inline line-item cell compares against, + * bound through `evalFieldPredicate`'s `scope` extra. + * + * NOT included, on purpose: `data`, `os`, `app`, `features`, `input`, `vars`, + * `page`. Those are real roots at some surfaces, but this builder never offers + * them, and over-capturing there fails in the WRONG direction — `data.csv` is + * a plausible literal, and `data` IS bound, so reading it as a reference would + * produce another silently-false predicate instead of a loud one. Which roots + * a mounting surface actually binds is caller-supplied vocabulary + * (objectui#6296) and is that card's to declare, not this one's to guess. + */ +const REFERENCE_ROOTS = ['record', 'previous', 'parent', 'user', 'current_user', 'org'] as const; + +/** + * A dotted path under a declared root — i.e. plainly a reference. + * + * Anchored, and dotted identifiers only. "Contains a dot" is NOT the test: a + * version string (`1.2.3`), a filename, and a path under a root nothing binds + * all stay literal text. + */ +const REFERENCE_RE = new RegExp( + `^(?:${REFERENCE_ROOTS.join('|')})(?:\\.[A-Za-z_][A-Za-z0-9_]*)+$`, +); + +/** + * Quote a raw value for CEL unless it is a number / boolean / null — or a + * reference (objectui#6293). + * + * Quoting a reference was silent in both directions: `previous == + * 'previous.status'` is valid CEL, `previous` is a declared root, and a string + * literal's contents are deliberately not scanned for references by + * `flow-ref-check` or by the server-side validator — so the predicate parsed, + * registered, evaluated, and was always false, with no author-time signal at + * any layer. Emitting the reference is also what makes it CHECKABLE: it is now + * an identifier those existing checkers can see. + */ function fmtValue(v: string): string { const t = v.trim(); if (t === 'true' || t === 'false' || t === 'null') return t; if (t !== '' && !Number.isNaN(Number(t))) return t; + if (REFERENCE_RE.test(t)) return t; return `'${t.replace(/'/g, "\\'")}'`; } -/** Inverse of fmtValue for display in the value input. */ +/** + * Inverse of fmtValue for display in the value input. A bare reference has no + * quotes to strip and passes through unchanged, which is what keeps an emitted + * `previous == previous.status` round-tripping back into the row builder. + */ function unfmtValue(raw: string): string { const t = raw.trim(); const m = /^'(.*)'$/.exec(t);