diff --git a/.changeset/27865-held-bad-name-one-message.md b/.changeset/27865-held-bad-name-one-message.md new file mode 100644 index 00000000000..04267ea9825 --- /dev/null +++ b/.changeset/27865-held-bad-name-one-message.md @@ -0,0 +1,13 @@ +--- +'@object-ui/app-shell': patch +'@object-ui/console': patch +--- + +fix(setup): a new metadata item whose name the editor already refuses is not sent, and one mistake is one message + +Clause-②: no +Metadata-protocol: none + +- **Before.** On Setup › Permission Sets › New (and every metadata create page), a Name the editor flagged as invalid was sent on Save anyway. The page then showed the server's refusal ("is not a legal metadata item name"), and after the next edit also the summary banner ("does not match the spec — 1 validation error(s)") and the inline error under Name: three messages for one mistake. +- **Now.** While the editor's own check refuses the Name of a new item, Save sends nothing and the error under Name is the message; a Name filled in from the Label, never touched, shows its error there too once Save is pressed. The summary banner no longer repeats an error the form already draws on its field, and a server refusal that names a field the form draws is said on that field only. +- **Unchanged.** Every other check the editor runs while you type stays advisory: Save still sends, and the server has the last word (an existing item is never held by it). Errors the form cannot draw on a field (the whole item, a nested key, a field off screen) are still listed in the banner. diff --git a/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.heldBadName-27865.test.tsx b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.heldBadName-27865.test.tsx new file mode 100644 index 00000000000..c6d89e4db95 --- /dev/null +++ b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.heldBadName-27865.test.tsx @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectstack#27865 — New Permission Set: one mistake is one message. + * + * (a) a Name the client validator refuses holds the save: no `PUT` leaves, + * even on a click inside the live pass's debounce, and the inline error + * on Name is the only message (no summary banner, no server refusal); + * (b) a Name the client accepts but the server refuses, naming `name`, is + * said once: inline on Name, with no banner repeating it; + * (c) a Name derived from the Label, never touched, is held the same way, + * and the Save attempt shows its error inline (the create form's + * show-on-touch deferral ends there); + * (d) a good Name saves (control); + * (e) the hold's BOUND: a good Name beside a client-refused key that is not + * the identity (the server-gained-a-key skew shape the advisory ruling + * is about) still sends — only the Name holds a create. + * + * The REAL page over a REAL `MetadataClient` whose transport is an in-memory + * server that records every `PUT` and answers the way the save door does: an + * ungrammatical name is a 400 `INVALID_REQUEST` with no issues + * (`refuseUngrammaticalMetaItemName`), and `taken_set` stands for a name the + * client cannot judge, refused 422 with an issue at `name`. + * + * That the live Zod pass stays advisory everywhere else is pinned, unchanged, + * in `ResourceEditPage.schemaAdvisory.test.tsx`. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import { z } from 'zod'; +import { getMetadataCreateSeed, getMetadataTypeSchema } from '@objectstack/spec/kernel'; +import { METADATA_ITEM_NAME_PATTERN } from '@objectstack/spec/shared'; +import { MetadataClient } from '@object-ui/data-objectstack'; + +const TAKEN = 'taken_set'; +const TAKEN_MESSAGE = 'A permission set with this name already exists.'; + +const server = { + puts: [] as Array<{ name: string; body: Record }>, +}; + +const json = (status: number, body: unknown) => + new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); + +async function serve(input: RequestInfo | URL, init?: RequestInit): Promise { + const method = (init?.method ?? 'GET').toUpperCase(); + const url = new URL(String(input), 'http://console.test'); + const meta = url.pathname.match(/^\/api\/v1\/meta\/(.+)$/); + if (!meta) return json(200, []); + const seg = meta[1].split('/').map(decodeURIComponent); + if (seg[0] === '_drafts') return json(200, { drafts: [] }); + if (seg.length === 1) return json(200, { items: [] }); + if (method === 'PUT') { + const name = seg[1]!; + server.puts.push({ name, body: JSON.parse(String(init?.body ?? '{}')) as Record }); + if (!METADATA_ITEM_NAME_PATTERN.test(name)) { + return json(400, { + success: false, + error: { code: 'INVALID_REQUEST', message: `${JSON.stringify(name)} is not a legal metadata item name.` }, + }); + } + if (name === TAKEN) { + return json(422, { + success: false, + error: { code: 'INVALID_METADATA', message: 'Invalid permission', details: { issues: [{ path: ['name'], message: TAKEN_MESSAGE }] } }, + }); + } + return json(200, { success: true, version: 'hmac-sha256:v1', seq: 1, state: 'draft', message: `Saved ${seg[0]}/${name}` }); + } + return json(404, { error: { code: 'NO_DRAFT', message: 'absent' } }); +} + +const client = vi.hoisted(() => ({ current: null as unknown })); +/** Extra keys the served create seed carries — set by (e) only. */ +const seedExtra = vi.hoisted(() => ({ current: {} as Record })); + +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => client.current, + useMetadataTypes: () => ({ + loading: false, + entries: [ + { + type: 'permission', + label: 'Permission Set', + domain: 'security', + allowEnvironmentOverlay: true, + allowRuntimeCreate: true, + schema: PERMISSION_JSON_SCHEMA, + createSeed: { ...(getMetadataCreateSeed('permission') as Record), ...seedExtra.current }, + }, + ], + }), + }; +}); + +import { MetadataResourceEditPage } from './ResourceEditPage'; +import { registerDefaultMetadataSchemas } from './default-schemas'; +import { registerBuiltinAnchors } from './anchors'; + +const PERMISSION_JSON_SCHEMA = z.toJSONSchema(getMetadataTypeSchema('permission') as z.ZodType, { + io: 'input', + unrepresentable: 'any', +}) as Record; + +client.current = new MetadataClient({ baseUrl: '', fetch: vi.fn(serve) as unknown as typeof fetch }); + +registerDefaultMetadataSchemas(); +// The live registration: Label → Name derive, `createLive`. +registerBuiltinAnchors(); + +beforeEach(() => { + server.puts = []; + seedExtra.current = {}; +}); +afterEach(() => { + cleanup(); +}); + +const saveButton = () => screen.getByTestId('resource-save'); + +async function openNewPermissionSet() { + render( + + + } /> + } /> + + , + ); + return screen.findByRole('textbox', { name: /^Name/ }, { timeout: 8000 }); +} + +/** The one message: exactly one inline issue, said nowhere else on the page. */ +async function expectOnlyInlineIssue(): Promise { + const inline = await screen.findAllByTestId('schema-form-issue', {}, { timeout: 8000 }); + expect(inline).toHaveLength(1); + const text = inline[0]!.textContent ?? ''; + expect(text).not.toBe(''); + expect(screen.getAllByText(text, { exact: false })).toHaveLength(1); + expect(screen.queryByTestId('metadata-validation-banner')).toBeNull(); + return text; +} + +describe('New Permission Set: one mistake, one message (objectstack#27865)', () => { + it('(a) a Name the client refuses holds the save — no PUT, only the inline error', async () => { + const nameBox = await openNewPermissionSet(); + fireEvent.change(nameBox, { target: { value: 'Bad Name' } }); + // Inside the live pass's 200 ms debounce: the hold must not depend on it. + fireEvent.click(saveButton()); + await expectOnlyInlineIssue(); + await new Promise((r) => setTimeout(r, 300)); + expect(server.puts).toEqual([]); + expect(screen.queryByText(/is not a legal metadata item name/)).toBeNull(); + expect(screen.queryByTestId('landed')).toBeNull(); + // A second click is held the same way. + fireEvent.click(saveButton()); + await new Promise((r) => setTimeout(r, 300)); + expect(server.puts).toEqual([]); + await expectOnlyInlineIssue(); + }); + + it('(b) a Name the client accepts but the server refuses at `name` is said once, inline', async () => { + const nameBox = await openNewPermissionSet(); + fireEvent.change(nameBox, { target: { value: TAKEN } }); + fireEvent.blur(nameBox); + fireEvent.click(saveButton()); + await waitFor(() => expect(server.puts).toHaveLength(1), { timeout: 8000 }); + const text = await expectOnlyInlineIssue(); + expect(text).toContain(TAKEN_MESSAGE); + expect(screen.queryByTestId('landed')).toBeNull(); + }); + + it('(c) a derived, untouched Name the client refuses is held and its error shown inline', async () => { + await openNewPermissionSet(); + const labelBox = screen.getByRole('textbox', { name: /^Label/ }); + fireEvent.change(labelBox, { target: { value: 'A' } }); + // Single-letter slug: refused by the spec's two-character floor. + await waitFor(() => expect(screen.getByRole('textbox', { name: /^Name/ })).toHaveValue('a')); + expect(screen.queryAllByTestId('schema-form-issue')).toHaveLength(0); + fireEvent.click(saveButton()); + await expectOnlyInlineIssue(); + await new Promise((r) => setTimeout(r, 300)); + expect(server.puts).toEqual([]); + }); + + it('(d) a good Name saves (control)', async () => { + const nameBox = await openNewPermissionSet(); + fireEvent.change(nameBox, { target: { value: 'sales_ops' } }); + fireEvent.click(saveButton()); + await waitFor(() => expect(server.puts).toHaveLength(1), { timeout: 8000 }); + expect(server.puts[0]!.name).toBe('sales_ops'); + await screen.findByTestId('landed', {}, { timeout: 8000 }); + }); + + it('(e) a good Name beside a client-refused non-identity key still sends — the hold is the Name only', async () => { + // A key the bundled spec does not declare, served in the create seed: the + // client's strict pass refuses it at the root, the (newer) server takes it. + seedExtra.current = { zzServerOnlyKey: true }; + const nameBox = await openNewPermissionSet(); + fireEvent.change(nameBox, { target: { value: 'sales_ops' } }); + // The client does refuse the draft — surfaced, advisory — and not at Name. + const banner = await screen.findByTestId('metadata-validation-banner', {}, { timeout: 8000 }); + expect(banner.textContent).toContain('(root)'); + expect(screen.queryAllByTestId('schema-form-issue')).toHaveLength(0); + fireEvent.click(saveButton()); + await waitFor(() => expect(server.puts).toHaveLength(1), { timeout: 8000 }); + expect(server.puts[0]!.name).toBe('sales_ops'); + expect(server.puts[0]!.body.zzServerOnlyKey).toBe(true); + await screen.findByTestId('landed', {}, { timeout: 8000 }); + }); +}); diff --git a/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.schemaAdvisory.test.tsx b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.schemaAdvisory.test.tsx index d08fb2522c2..cae6aa22da2 100644 --- a/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.schemaAdvisory.test.tsx +++ b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.schemaAdvisory.test.tsx @@ -45,6 +45,19 @@ * ⛔ Neither suite may be cited without the other. A carve-out with only its * blocking half pinned is indistinguishable from having deleted this one. * + * ## ⭐ The create-name carve-out, objectstack#27865 — also read with this suite + * + * One PREDICTED verdict does hold a save, and it is bounded: on the create + * door only, when the live pass refuses the identity field (the Name), + * `doSave` sends nothing and the error stays on that field. The ruling's + * reason does not reach it: the create form always offers the Name input, so a + * client stricter than the server costs the author a different name, never a + * dead-bolt. Both halves are pinned in + * `ResourceEditPage.heldBadName-27865.test.tsx` — the hold, and its bound: a + * good Name beside a client-refused non-identity key (this suite's skew shape, + * on the create door) still sends. Every case below is unchanged by it: an + * edit is never held, and no other live issue holds a create. + * * ## The fixture is the real skew shape, not a contrived one * * The stored page carries `zzServerOnlyKey` — a key the bundled diff --git a/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index d6f6691741c..82de52989a5 100644 --- a/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/ui/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -629,6 +629,16 @@ function MetadataResourceEditPageImpl({ // the user types anything reads as broken, not helpful (the save path still // validates). Flips true on the first real edit. const [createDirty, setCreateDirty] = React.useState(false); + // objectstack#27865 — one message per mistake. `revealIssues` ends the create + // form's show-on-touch deferral once Save is attempted; `inlineIssueFields` + // is what the form reports it draws inline (a top-level field, mounted, not + // deferred), which the summary banner and the refusal line leave out. + const [revealIssues, setRevealIssues] = React.useState(false); + const [inlineIssueFields, setInlineIssueFields] = React.useState>(() => new Set()); + const inlineIssueFieldsRef = React.useRef(inlineIssueFields); + React.useEffect(() => { + inlineIssueFieldsRef.current = inlineIssueFields; + }); // Wrap setDraft so that editing a field clears any *server-side* // diagnostic issues whose path begins with that field. The user @@ -946,7 +956,10 @@ function MetadataResourceEditPageImpl({ // // So schema issues are surfaced loudly — banner, inline field errors, // `previewDiagnostics` — and left advisory; the server has the last word. - // Pinned by `ResourceEditPage.schemaAdvisory.test.tsx`. + // Pinned by `ResourceEditPage.schemaAdvisory.test.tsx`. One bounded + // exception, outside this gate: a create whose NAME the live pass refuses is + // held in `doSave` (objectstack#27865) — the create form always offers that + // input, so no skew can wedge it. // // ⭐ THE BOUNDARY, stated so a later reader can apply it (`9073cf018`). // "Advisory" is about a verdict this client PREDICTS, and it is the whole of @@ -1810,6 +1823,10 @@ function MetadataResourceEditPageImpl({ setSaving(true); setError(null); setIssues([]); + if (createMode) { + setRevealIssues(true); + setCreateDirty(true); + } try { // Ensure identity is set on create, and that any `createDefaults` // / `createBuildBody` shape (e.g. `{ fields: {} }` for object, @@ -1852,6 +1869,31 @@ function MetadataResourceEditPageImpl({ setSaving(false); return; } + // objectstack#27865 — a create whose NAME the client validator refuses is + // held: no request, and the refusal is the inline error on the Name field + // (revealed above). The verdict is the live pass's own, run here on the + // same draft so a Save inside its debounce cannot slip past it. + // + // ⭐ This is the one place a predicted verdict holds Save, and it is + // bounded so the dead-bolt the advisory ruling at `blockingReport` + // guards against stays out of reach: only the create door, and only the + // identity field — an input the create form always offers, so a client + // stricter than the server can never wedge the author, who can always + // type a name both accept. Every other live issue stays advisory, and an + // edit is never held here. + if (createMode && hasClientValidator(type, 'create')) { + const verdict = await validateMetadataDraft( + type, + draft, + entry?.schema as { required?: unknown } | undefined, + { mode: 'create' }, + ); + if (verdict.issues.some((issue) => issue.path === identityField)) { + setIssues(verdict.issues); + setSaving(false); + return; + } + } // objectui#12810 — a create never saves the seed's placeholder identity // (`buildCreateModeBody` leaves it out). One the author left unset that // the type requires — a dataset's base object — is refused HERE, named, @@ -2023,7 +2065,16 @@ function MetadataResourceEditPageImpl({ held, summary: held.map((h) => h.label).join(', '), }); - if (mapped.length === 1 && !mapped[0].path) { + // objectstack#27865 — a refusal the form draws in full on its fields is + // said there, once. Only where the summary banner reads these issues + // (a client validator exists), so a field that leaves the screen still + // has its message somewhere. + const drawnInline = + hasClientValidator(type, draftMode) && + mapped.every((m) => !!m.path && inlineIssueFieldsRef.current.has(m.path)); + if (drawnInline) { + setError(null); + } else if (mapped.length === 1 && !mapped[0].path) { setError(mapped[0].message); } else if (mapped.length === 1) { setError(`${labelForIssuePath(mapped[0].path)}: ${mapped[0].message}`); @@ -3027,7 +3078,11 @@ function MetadataResourceEditPageImpl({ ) { return null; } - const errs = liveErrors; + // objectstack#27865 — an error the form already draws on its + // field is not repeated here. + const errs = liveErrors.filter( + (e) => !(inlineIssueFields.has(e.path) && displayIssues.some((d) => d.path === e.path)), + ); const warns = diag?.warnings ?? []; const hasErrs = !liveValid && errs.length > 0; const hasWarns = warns.length > 0; @@ -3403,6 +3458,8 @@ function MetadataResourceEditPageImpl({ readOnly={formReadOnly} createMode={createMode} widgetContext={widgetContext} + revealIssues={revealIssues} + onInlineIssueFieldsChange={setInlineIssueFields} /> )} @@ -3427,6 +3484,8 @@ function MetadataResourceEditPageImpl({ readOnly={formReadOnly} createMode={createMode} widgetContext={widgetContext} + revealIssues={revealIssues} + onInlineIssueFieldsChange={setInlineIssueFields} /> )} {/* objectui#7611 — under the designer, too: `position` renders diff --git a/ui/packages/app-shell/src/views/metadata-admin/SchemaForm.tsx b/ui/packages/app-shell/src/views/metadata-admin/SchemaForm.tsx index c6f9eb717d0..5d94e31a1e2 100644 --- a/ui/packages/app-shell/src/views/metadata-admin/SchemaForm.tsx +++ b/ui/packages/app-shell/src/views/metadata-admin/SchemaForm.tsx @@ -1123,6 +1123,17 @@ export interface SchemaFormProps { createMode?: boolean; /** Out-of-band data widgets need (object list, etc). */ widgetContext?: WidgetContext; + /** + * Show every field's issues now, touched or not: the host's Save was + * attempted, so the deferral below has run its course (objectstack#27865). + */ + revealIssues?: boolean; + /** + * Told which TOP-LEVEL fields of this form currently draw their issues + * inline: mounted and not deferred. The host's summary banner leaves those + * out, so one mistake is one message (objectstack#27865). + */ + onInlineIssueFieldsChange?: (fields: ReadonlySet) => void; } function SchemaFormBody({ @@ -1373,6 +1384,45 @@ const SchemaFormNestingContext = React.createContext(false); */ const DeferIssueDisplayContext = React.createContext(false); +/** + * The outermost form's register of the top-level fields that draw their + * issues inline (objectstack#27865). A row registers while it is mounted and + * shows its issue line; a nested form provides `null`, so only the host + * form's own rows count. The value is held in state, never memoised: rows key + * their effect on it (AGENTS.md #10). + */ +interface InlineIssueFieldRegister { + /** The id path of the form's own top-level rows. */ + rootIdPath: string | undefined; + register: (field: string) => () => void; +} +const InlineIssueFieldsContext = React.createContext(null); + +function useInlineIssueFieldRegister( + rootIdPath: string | undefined, + onChange: SchemaFormProps['onInlineIssueFieldsChange'], +): InlineIssueFieldRegister { + const onChangeRef = React.useRef(onChange); + React.useEffect(() => { + onChangeRef.current = onChange; + }); + const [counts] = React.useState(() => new Map()); + const [register] = React.useState(() => (field: string) => { + const publish = () => onChangeRef.current?.(new Set(counts.keys())); + counts.set(field, (counts.get(field) ?? 0) + 1); + publish(); + return () => { + const n = (counts.get(field) ?? 1) - 1; + if (n > 0) counts.set(field, n); + else counts.delete(field); + publish(); + }; + }); + const [value, setValue] = React.useState(() => ({ rootIdPath, register })); + if (value.rootIdPath !== rootIdPath) setValue({ rootIdPath, register }); + return value; +} + const isDevBuild = (): boolean => (globalThis as { process?: { env?: Record } }).process?.env ?.NODE_ENV !== 'production'; @@ -1428,12 +1478,16 @@ export function SchemaForm(props: SchemaFormProps) { const inheritedDefer = React.useContext(DeferIssueDisplayContext); useDuplicateFieldHostIdCheck(!nested); // `add10d8f9` — a create form defers issue DISPLAY to first touch; a - // nested form inherits whatever the form hosting it decided. - const deferIssues = props.createMode === true || inheritedDefer; + // nested form inherits whatever the form hosting it decided. A Save attempt + // ends the deferral (objectstack#27865). + const deferIssues = !props.revealIssues && (props.createMode === true || inheritedDefer); + const inlineRegister = useInlineIssueFieldRegister(props.idPath, props.onInlineIssueFieldsChange); return ( - + + + ); @@ -1808,6 +1862,14 @@ function FieldRow({ const [touched, setTouched] = React.useState(false); const markTouched = React.useCallback(() => setTouched(true), []); const visibleIssues = !deferIssues || touched ? issues : undefined; + // objectstack#27865 — a host-level row that shows its issue line tells the + // host, whose summary banner then leaves that field out. + const inlineRegister = React.useContext(InlineIssueFieldsContext); + const showsIssues = !deferIssues || touched; + React.useEffect(() => { + if (!inlineRegister || idPath !== inlineRegister.rootIdPath || !showsIssues) return; + return inlineRegister.register(name); + }, [inlineRegister, idPath, name, showsIssues]); const handleChange = React.useCallback( (v: unknown) => { setTouched(true);