Skip to content

Commit 8e02ad7

Browse files
fix(app-shell): page block inspector authors visibleWhen, not hidden (#3229) (#3286)
The conditional-visibility `ConditionBuilder` on the selected page block read and wrote `block.hidden`. `PageComponentSchema` (`ui/page.zod.ts`) is `.strict()` and has no `hidden` key, so the designer — a PRODUCER — was emitting drafts guaranteed to fail parse on save, naming a key the author never typed. The canonical key is `visibleWhen` (ADR-0089). The control now reads/writes `visibleWhen` through the shared expression envelope pair (#3218), since `visibleWhen` is `ExpressionInputSchema` and a persisted block carries `{ dialect, source }` rather than the authored string. The semantic flip lands in the UI copy too — `hidden` and `visibleWhen` are inverses, so the label and the i18n key move with it (`engine.inspector.pageBlock.hidden` → `.visibleWhen`, both language packs: "Hidden (CEL)" → "Visible when (CEL)", 隐藏条件 → 显示条件). Renaming the key under hide-flavoured copy would make authors write predicates backwards — metadata that parses and means the opposite (the #3276 class, which #3257's parse guard is structurally blind to). No value is migrated: negating an arbitrary CEL predicate textually is unsound, and since ADR-0089 D3a no valid stored draft can carry `hidden` anyway. Tests pin DIRECTION, not just the key name: the block the inspector actually commits is rendered through SchemaRenderer and must appear when its predicate is true and be absent when false. Claude-Session: https://claude.ai/code/session_01NVPjPzmmAJ2Ngtvgg5MSRa Co-authored-by: Claude <noreply@anthropic.com>
1 parent a8aa576 commit 8e02ad7

4 files changed

Lines changed: 339 additions & 6 deletions

File tree

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
"@object-ui/app-shell": patch
3+
---
4+
5+
The Page block inspector's conditional-visibility control now authors
6+
`visibleWhen`, and says "Visible when" while doing it (objectui#3229).
7+
8+
FROM: the `ConditionBuilder` rendered for the selected page block read and wrote
9+
`block.hidden`. TO: it reads and writes `block.visibleWhen`, the canonical
10+
conditional-visibility key (ADR-0089), through the same envelope read/write pair
11+
the hook and action guards use (objectui#3218).
12+
13+
`PageComponentSchema` (`ui/page.zod.ts`) is `.strict()` and has no `hidden` key
14+
at all. So this was not a tolerant consumer accepting something sloppy — it was
15+
the **producer emitting a key the contract rejects**: every author who filled
16+
that box got a save-time parse failure naming a key they never typed, because the
17+
inspector typed it for them. The spec's own error message already names the fix
18+
(`` the canonical key is `visibleWhen` ``). A designer that mass-produces drafts
19+
guaranteed to be rejected is a worse failure than a lenient reader — a lenient
20+
reader lets wrong metadata run, this made the *correct tool* emit the wrong thing.
21+
22+
The fix also closes the gap in the other direction: a **valid** block carrying
23+
`visibleWhen` previously had no control in the inspector that could edit its
24+
visibility, because the only visibility control on screen wrote a different key.
25+
Removing the control instead would have stopped teaching the wrong key but left
26+
that gap open, which is a capability regression.
27+
28+
**The semantic flip lands in the UI copy, not just the key.** `hidden` and
29+
`visibleWhen` are inverses (`hidden` true ⇒ gone; `visibleWhen` true ⇒ shown), so
30+
the label, the i18n key (`engine.inspector.pageBlock.hidden`
31+
`engine.inspector.pageBlock.visibleWhen`) and both language packs move together:
32+
"Hidden (CEL)" → "Visible when (CEL)", 「隐藏条件(CEL)」→「显示条件(CEL)」.
33+
Renaming the key while leaving hide-flavoured copy would have been worse than
34+
leaving the bug: authors would write the predicate backwards, producing metadata
35+
that PARSES and means the opposite — the objectui#3276 class, which objectui#3257's
36+
guard is structurally blind to because it only asks whether a draft parses.
37+
38+
**No value is migrated.** An existing `hidden` expression is NOT negated into
39+
`visibleWhen`: textually negating an arbitrary CEL predicate is unsound
40+
(`!(a && b)` is not `!a && !b`), and there is no stored valid draft to migrate
41+
anyway — since ADR-0089 D3a `hidden` is a loud parse failure on save, and before
42+
D3a the key was silently dropped, so it never reached published metadata either.
43+
A loud error on a rare stale draft, with the spec's message pointing at
44+
`visibleWhen`, beats silently rewriting an author's predicate.
45+
46+
Because `visibleWhen` is `ExpressionInputSchema`, a persisted block carries the
47+
`{ dialect, source }` envelope rather than the authored string, so the control
48+
goes through `expressionSource` / `writeExpressionSource` — an edit preserves
49+
`dialect` and `meta` and drops the stale `ast`, instead of flattening the
50+
envelope to a bare string and silently swapping the evaluation engine.
51+
52+
Tests pin DIRECTION, not just the key name (the objectui#3276 precedent makes
53+
that a hard requirement): the block the inspector actually commits is rendered
54+
through `SchemaRenderer` and must appear when its predicate is true and be absent
55+
when it is false, alongside assertions that the committed key is `visibleWhen`,
56+
never `hidden`, and that the resulting draft parses.

packages/app-shell/src/views/metadata-admin/i18n.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,7 @@ const ENGINE_STRINGS_EN: Record<string, string> = {
473473
'engine.inspector.pageBlock.type': 'Type',
474474
'engine.inspector.pageBlock.id': 'ID',
475475
'engine.inspector.pageBlock.className': 'Class names',
476-
'engine.inspector.pageBlock.hidden': 'Hidden (CEL)',
476+
'engine.inspector.pageBlock.visibleWhen': 'Visible when (CEL)',
477477
'engine.inspector.pageBlock.properties': 'Properties',
478478
'engine.inspector.pageBlock.advanced': 'Advanced',
479479
'engine.inspector.pageBlock.remove': 'Remove block',
@@ -2032,7 +2032,7 @@ const ENGINE_STRINGS_ZH: Record<string, string> = {
20322032
'engine.inspector.pageBlock.type': '类型',
20332033
'engine.inspector.pageBlock.id': 'ID',
20342034
'engine.inspector.pageBlock.className': '类名',
2035-
'engine.inspector.pageBlock.hidden': '隐藏条件(CEL)',
2035+
'engine.inspector.pageBlock.visibleWhen': '显示条件(CEL)',
20362036
'engine.inspector.pageBlock.properties': '属性',
20372037
'engine.inspector.pageBlock.advanced': '高级属性',
20382038
'engine.inspector.pageBlock.remove': '删除区块',

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

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
import * as React from 'react';
1515
import type { MetadataInspectorProps } from '../inspector-registry';
16+
import type { ExpressionInput } from '@objectstack/spec/shared';
1617
import { t } from '../i18n';
1718
import {
1819
InspectorShell,
@@ -28,6 +29,7 @@ import {
2829
import { BLOCK_CONFIG, blockHasConfig, type BlockPropField } from '../previews/block-config';
2930
import { ColorVariantPicker } from '../color-variant-field';
3031
import { ConditionBuilder } from './ConditionBuilder';
32+
import { expressionSource, writeExpressionSource } from './expression-envelope';
3133
import { useObjectOptions } from '../previews/useObjectOptions';
3234
import { useObjectFields } from '../previews/useObjectFields';
3335
import {
@@ -190,7 +192,15 @@ interface Block {
190192
type?: string;
191193
id?: string;
192194
className?: string;
193-
hidden?: string;
195+
/**
196+
* Conditional visibility, SHOW-when-truthy (ADR-0089). This is the only key
197+
* `PageComponentSchema` (`.strict()`) accepts for it — `hidden` is not in the
198+
* key set at all, so a block carrying it is a loud parse failure on save
199+
* (objectui#3229). `ExpressionInput`, not `string`: the spec normalizes an
200+
* authored string into `{ dialect, source }`, so a persisted block carries
201+
* the envelope — read/write it through the shared pair.
202+
*/
203+
visibleWhen?: ExpressionInput;
194204
children?: Block[];
195205
[k: string]: unknown;
196206
}
@@ -550,10 +560,24 @@ export function PageBlockInspector({ selection, draft, onPatch, onClearSelection
550560
<InspectorTextField label={t('engine.inspector.pageBlock.type', locale)} value={block.type ?? ''} onCommit={(v) => patch({ type: v })} disabled={readOnly} mono />
551561
<InspectorTextField label={t('engine.inspector.pageBlock.id', locale)} value={block.id ?? ''} onCommit={(v) => patch({ id: v })} disabled={readOnly} mono />
552562
<InspectorTextField label={t('engine.inspector.pageBlock.className', locale)} value={block.className ?? ''} onCommit={(v) => patch({ className: v })} disabled={readOnly} mono />
563+
{/* Conditional visibility. The key is `visibleWhen` and the label says
564+
"Visible when" — the two must move together (objectui#3229). This
565+
control used to author `hidden`, a key `PageComponentSchema`
566+
(`.strict()`) does not have: the designer was mass-producing drafts
567+
the spec rejects on save, naming a key the author never typed.
568+
Renaming the key alone would have been worse than leaving it — a
569+
"hide when" label over a show-when-truthy key makes authors write
570+
the predicate backwards, i.e. metadata that PARSES and means the
571+
opposite (the objectui#3276 class, which #3257's parse guard is
572+
structurally blind to). No value is migrated: negating an arbitrary
573+
CEL predicate textually is unsound (`!(a && b)` is not `!a && !b`),
574+
and the spec's parse error already names `visibleWhen` as the fix.
575+
`visibleWhen` is `ExpressionInputSchema`, so it goes through the same
576+
envelope read/write pair as the hook / action guards (#3218). */}
553577
<ConditionBuilder
554-
label={t('engine.inspector.pageBlock.hidden', locale)}
555-
value={block.hidden ?? ''}
556-
onCommit={(v) => patch({ hidden: v || undefined })}
578+
label={t('engine.inspector.pageBlock.visibleWhen', locale)}
579+
value={expressionSource(block.visibleWhen)}
580+
onCommit={(v) => patch({ visibleWhen: writeExpressionSource(block.visibleWhen, v) })}
557581
objectName={pageObject}
558582
disabled={readOnly}
559583
/>
Lines changed: 253 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,253 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* objectui#3229 — the Page block inspector's conditional-visibility control
5+
* must author `visibleWhen`, and must SAY "visible when" while doing it.
6+
*
7+
* The control used to read/write `block.hidden`. `PageComponentSchema`
8+
* (`ui/page.zod.ts`) is `.strict()` and has no `hidden` key at all, so the
9+
* designer — a PRODUCER — was emitting drafts guaranteed to be rejected on
10+
* save, with a parse error naming a key the author never typed. The canonical
11+
* key is `visibleWhen` (ADR-0089); `visibility` survives as a deprecated alias.
12+
*
13+
* ## Why direction, not just the key name, is pinned here
14+
*
15+
* `hidden` and `visibleWhen` are INVERSES (`hidden` true ⇒ gone,
16+
* `visibleWhen` true ⇒ shown). Renaming the key while leaving "hidden"-flavoured
17+
* copy on screen would have produced metadata that PARSES and means the
18+
* opposite — the objectui#3276 class, which #3257's guard is structurally blind
19+
* to because it only asks whether a draft parses. So these tests assert the
20+
* rendered OUTCOME of the inspector's own output, in both directions, rather
21+
* than just `expect(patch).toHaveProperty('visibleWhen')`.
22+
*
23+
* FIXTURE DISCIPLINE (objectui#3216's method): no envelope is hand-written.
24+
* Every fixture is the AUTHORED input fed through `PageSchema.parse`, so a
25+
* fixture cannot drift from the spec — `visibleWhen` is `ExpressionInputSchema`,
26+
* which normalizes a bare string into `{ dialect: 'cel', source }`.
27+
*/
28+
29+
import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest';
30+
import { render, screen, fireEvent, cleanup } from '@testing-library/react';
31+
import { PageSchema } from '@objectstack/spec/ui';
32+
import { ComponentRegistry } from '@object-ui/core';
33+
import { SchemaRenderer, PredicateScopeProvider } from '@object-ui/react';
34+
import { PageBlockInspector } from './PageBlockInspector';
35+
36+
afterEach(cleanup);
37+
38+
/** Selection id for the single block in the fixture page. */
39+
const BLOCK_PATH = 'regions[0].components[0]';
40+
41+
/**
42+
* Author a page the way a user does, parse it with the spec, and hand the
43+
* RESULT to the inspector — exactly what the metadata editor loads.
44+
*/
45+
function pageDraft(block: Record<string, unknown>): Record<string, unknown> {
46+
return PageSchema.parse({
47+
name: 'home',
48+
label: 'Home',
49+
type: 'home',
50+
template: 'default',
51+
regions: [{ name: 'main', components: [{ type: 'text', id: 'b1', ...block }] }],
52+
}) as unknown as Record<string, unknown>;
53+
}
54+
55+
function renderInspector(
56+
draft: Record<string, unknown>,
57+
{ locale = 'en-US', onPatch = vi.fn() }: { locale?: string; onPatch?: ReturnType<typeof vi.fn> } = {},
58+
) {
59+
render(
60+
<PageBlockInspector
61+
type="page"
62+
name="home"
63+
draft={draft}
64+
selection={{ kind: 'block', id: BLOCK_PATH }}
65+
onPatch={onPatch}
66+
onClearSelection={() => {}}
67+
readOnly={false}
68+
locale={locale as never}
69+
/>,
70+
);
71+
return onPatch;
72+
}
73+
74+
/**
75+
* The block as the inspector last wrote it. `onPatch` carries a SHALLOW patch
76+
* (`{ regions }`), not the whole draft.
77+
*/
78+
function committedBlock(onPatch: ReturnType<typeof vi.fn>): Record<string, unknown> {
79+
const patch = onPatch.mock.calls.at(-1)![0] as any;
80+
return patch.regions[0].components[0] as Record<string, unknown>;
81+
}
82+
83+
/** What the editor would actually save: the shallow patch applied to the draft. */
84+
function savedDraft(
85+
draft: Record<string, unknown>,
86+
onPatch: ReturnType<typeof vi.fn>,
87+
): Record<string, unknown> {
88+
return { ...draft, ...(onPatch.mock.calls.at(-1)![0] as Record<string, unknown>) };
89+
}
90+
91+
/** The no-code builder's value box for the single parsed row. */
92+
const rowValueInput = () => screen.getByPlaceholderText('value') as HTMLInputElement;
93+
/** The raw-expression editor (CelPredicateField renders a combobox TEXTAREA). */
94+
const rawEditor = () =>
95+
screen.getAllByRole('combobox').find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement;
96+
97+
/* ───────────────────────── the key that is committed ───────────────────── */
98+
99+
describe('PageBlockInspector — conditional visibility authors `visibleWhen` (#3229)', () => {
100+
it('reads the `source` of a persisted envelope into the control', () => {
101+
const draft = pageDraft({ visibleWhen: 'record.amount > 10' });
102+
// Pin what the platform actually stores, so this fails loudly if the spec
103+
// stops normalizing rather than passing on a stale assumption.
104+
expect((draft as any).regions[0].components[0].visibleWhen).toEqual({
105+
dialect: 'cel',
106+
source: 'record.amount > 10',
107+
});
108+
109+
renderInspector(draft);
110+
111+
// The builder adopted the predicate: its value box carries `10` and the
112+
// compiled preview echoes the whole thing.
113+
expect(rowValueInput().value).toBe('10');
114+
expect(screen.getByText('record.amount > 10')).toBeInTheDocument();
115+
});
116+
117+
it('commits `visibleWhen` — and never `hidden`', () => {
118+
const onPatch = renderInspector(pageDraft({ visibleWhen: 'record.amount > 10' }));
119+
120+
fireEvent.change(rowValueInput(), { target: { value: '20' } });
121+
122+
const block = committedBlock(onPatch);
123+
expect(block).toHaveProperty('visibleWhen');
124+
// The whole point of the issue: the designer must not type a key the
125+
// `.strict()` page schema does not have.
126+
expect(block).not.toHaveProperty('hidden');
127+
expect(block.visibleWhen).toEqual({ dialect: 'cel', source: 'record.amount > 20' });
128+
});
129+
130+
it('the committed draft PARSES — the shape the old `hidden` key could not', () => {
131+
const draft = pageDraft({ visibleWhen: 'record.amount > 10' });
132+
const onPatch = renderInspector(draft);
133+
fireEvent.change(rowValueInput(), { target: { value: '20' } });
134+
135+
// What the author would be saving. Before #3229 this threw
136+
// `unrecognized_keys … ["hidden"]`.
137+
expect(() => PageSchema.parse(savedDraft(draft, onPatch))).not.toThrow();
138+
139+
// And the counter-proof that the strictness under test is real.
140+
expect(() => pageDraft({ hidden: 'record.amount > 20' })).toThrow(/hidden/);
141+
});
142+
143+
it('authoring from scratch commits the bare-string shorthand under `visibleWhen`', () => {
144+
const draft = pageDraft({});
145+
expect((draft as any).regions[0].components[0].visibleWhen).toBeUndefined();
146+
const onPatch = renderInspector(draft);
147+
148+
// No prior value ⇒ the builder opens in row mode; switch to the raw editor
149+
// to author a predicate without driving Radix selects.
150+
fireEvent.click(screen.getByText('Expression'));
151+
fireEvent.change(rawEditor(), { target: { value: 'record.amount > 20' } });
152+
153+
const block = committedBlock(onPatch);
154+
expect(block.visibleWhen).toBe('record.amount > 20');
155+
expect(block).not.toHaveProperty('hidden');
156+
// The spec's pipe normalizes the shorthand to exactly the envelope.
157+
expect((PageSchema.parse(savedDraft(draft, onPatch)) as any).regions[0].components[0].visibleWhen)
158+
.toEqual({ dialect: 'cel', source: 'record.amount > 20' });
159+
});
160+
161+
it('preserves a non-`cel` dialect and `meta` across an edit (#3218 write rule)', () => {
162+
const draft = pageDraft({
163+
visibleWhen: { dialect: 'cel', source: 'record.amount > 10', meta: { rationale: 'Large deals only' } },
164+
});
165+
const onPatch = renderInspector(draft);
166+
167+
fireEvent.change(rowValueInput(), { target: { value: '20' } });
168+
169+
expect(committedBlock(onPatch).visibleWhen).toEqual({
170+
dialect: 'cel',
171+
source: 'record.amount > 20',
172+
meta: { rationale: 'Large deals only' },
173+
});
174+
});
175+
176+
it('clears the predicate to `undefined` when the author empties it', () => {
177+
const onPatch = renderInspector(pageDraft({ visibleWhen: 'record.amount > 10' }));
178+
179+
fireEvent.click(screen.getByLabelText('Remove condition'));
180+
181+
expect(committedBlock(onPatch).visibleWhen).toBeUndefined();
182+
});
183+
});
184+
185+
/* ─────────────────────────── the UI copy (semantics) ───────────────────── */
186+
187+
describe('PageBlockInspector — the label says "visible", not "hidden" (#3229)', () => {
188+
it('labels the control "Visible when" in English', () => {
189+
renderInspector(pageDraft({}));
190+
expect(screen.getByText(/Visible when/i)).toBeInTheDocument();
191+
// A "hide when" label over a show-when-truthy key is how authors come to
192+
// write the predicate backwards (the #3276 defect class).
193+
expect(screen.queryByText(/^Hidden/i)).not.toBeInTheDocument();
194+
});
195+
196+
it('labels the control 显示条件 in Chinese', () => {
197+
renderInspector(pageDraft({}), { locale: 'zh-CN' });
198+
expect(screen.getByText(//)).toBeInTheDocument();
199+
expect(screen.queryByText(//)).not.toBeInTheDocument();
200+
});
201+
});
202+
203+
/* ──────────────────────────────── DIRECTION ────────────────────────────── */
204+
205+
/**
206+
* The acceptance criterion the issue calls a hard requirement: what the
207+
* inspector writes must MEAN "visible when". These render the inspector's own
208+
* committed block through `SchemaRenderer` — the renderer `PageBlockCanvas`
209+
* hands blocks to — and assert both directions.
210+
*/
211+
describe('PageBlockInspector — `visibleWhen` direction is show-when-truthy (#3229)', () => {
212+
const Block = () => <div data-testid="block">Block</div>;
213+
214+
beforeEach(() => {
215+
ComponentRegistry.register('text', Block);
216+
});
217+
afterEach(() => {
218+
ComponentRegistry.unregister?.('text');
219+
});
220+
221+
/** Author `record.amount > 20` through the inspector and return the block. */
222+
function authoredBlock(): Record<string, unknown> {
223+
const onPatch = renderInspector(pageDraft({ visibleWhen: 'record.amount > 10' }));
224+
fireEvent.change(rowValueInput(), { target: { value: '20' } });
225+
const block = committedBlock(onPatch);
226+
cleanup();
227+
return block;
228+
}
229+
230+
function renderBlock(block: Record<string, unknown>, record: Record<string, unknown>) {
231+
return render(
232+
<PredicateScopeProvider scope={{ record }}>
233+
<SchemaRenderer schema={block as never} />
234+
</PredicateScopeProvider>,
235+
);
236+
}
237+
238+
it('renders the block when the authored predicate is TRUE', () => {
239+
renderBlock(authoredBlock(), { amount: 30 });
240+
expect(screen.getByTestId('block')).toBeInTheDocument();
241+
});
242+
243+
it('does NOT render the block when the authored predicate is FALSE', () => {
244+
const { container } = renderBlock(authoredBlock(), { amount: 5 });
245+
expect(screen.queryByTestId('block')).not.toBeInTheDocument();
246+
expect(container.innerHTML).toBe('');
247+
});
248+
249+
it('renders the block when there is no predicate at all', () => {
250+
renderBlock({ type: 'text', id: 'b1' }, { amount: 5 });
251+
expect(screen.getByTestId('block')).toBeInTheDocument();
252+
});
253+
});

0 commit comments

Comments
 (0)