Skip to content

Commit 0ded602

Browse files
authored
fix(form): a server rejection that names fields now marks those fields (#2966)
The server's VALIDATION_FAILED always carried fields[] — one entry per offending field with a human message — and every form dropped it, showing one undirected toast. On a long form the offending field is usually off-screen, so Save appeared to do nothing. Three layers, each of which was dropping the detail: @object-ui/react gains extractFieldErrors(err), normalising the three shapes the error arrives in (typed ValidationError, the raw client error whose details falls back to the whole response body, and the hand-rolled duck-typed shape); @object-ui/data-objectstack maps a 400 VALIDATION_FAILED onto the ValidationError class that had been exported and never once constructed, and `create` now normalises at all (only `update` did); the form renderer applies entries via form.setError — but only when every rejected field has a visible input, falling through to the banner otherwise so the part the user cannot see inline is still said out loud. The toast + scroll-and-focus of the first offender is shared with the client-side invalid handler (announceFieldErrors): to the person filling in the form these are the same event — only the referee differs. Removes the reason for client-side predicate mirroring (#2962): a form no longer has to guess what the server will reject. Non-field failures (403, permission denials) take exactly the path they took before. 14 + 6 new tests; 107 files / 1073 tests green; tsc clean on all three changed packages.
1 parent 52ec79d commit 0ded602

8 files changed

Lines changed: 599 additions & 41 deletions

File tree

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
---
2+
"@object-ui/react": minor
3+
"@object-ui/data-objectstack": minor
4+
"@object-ui/components": minor
5+
---
6+
7+
fix(form): a server rejection that names fields now marks those fields (objectstack#3896)
8+
9+
The server has always said which field it rejected. `@objectstack/objectql`'s
10+
validators throw `VALIDATION_FAILED` with `fields[]` — one entry per offending
11+
field, each with a human `message` — and both the REST layer and the runtime
12+
dispatcher serve that as a 400 with the entries intact.
13+
14+
Every form dropped them. The submit handler caught the rejection, ran the
15+
message through `extractWriteErrorMessage`, and showed **one undirected toast**:
16+
the user was told something was wrong but not *what*, on a surface that already
17+
knows how to mark an input — and already does exactly that for client-side
18+
validation. On a long form the offending field was often off-screen, so "创建"
19+
appeared to do nothing.
20+
21+
**Now the two failures behave identically, because they share one
22+
implementation.** The per-field marking, the toast naming the fields, and the
23+
scroll-and-focus of the first offender (#2793) were extracted from the
24+
client-side invalid handler; the server path calls the same function. As far as
25+
the person filling in the form is concerned these are the same event — only the
26+
referee differs.
27+
28+
Three layers, each of which was dropping the detail:
29+
30+
- **`@object-ui/react`** — new `extractFieldErrors(err)` (exported alongside
31+
`extractWriteErrorMessage` / `isPermissionError`) normalises the three shapes
32+
the error can arrive in: a typed `ValidationError` from the ObjectStack
33+
adapter, the raw `@objectstack/client` error (whose `details` falls back to the
34+
whole response body, which is where `fields[]` lands), and a hand-rolled error
35+
carrying `fields` directly — the server duck-types that shape identically, so
36+
the client must not be pickier than the server. Entries with no usable `field`
37+
are **dropped rather than guessed at**: marking an innocent input is worse than
38+
the generic toast.
39+
- **`@object-ui/data-objectstack`**`normaliseClientError` now maps a 400
40+
`VALIDATION_FAILED` onto the `ValidationError` class that has sat in
41+
`errors.ts` since the package was written, exported and **never once
42+
constructed**. Its `validationErrors: Array<{ field, message }>` shape was
43+
already exactly right. `create` also now normalises at all: only `update` did,
44+
so a rejected insert reached callers as the raw client error — and a create is
45+
the path that most often trips required-field validation.
46+
- **`@object-ui/components`** — the form renderer maps the entries onto
47+
`form.setError` and takes over the failure, **but only when every rejected
48+
field has a visible input to carry it**. If the server also rejected something
49+
the form does not render, it falls through to the banner, whose top-level
50+
message concatenates every field's reason — so the part the user cannot see
51+
inline is still said out loud instead of silently dropped.
52+
53+
This also removes the need for the client-side predicate mirroring added in
54+
#2962: a form no longer has to guess what the server will reject in order to
55+
warn about it beforehand, and mirrored predicates drift.
56+
57+
Non-field failures (403 / permission denials / anything without `fields[]`) take
58+
exactly the path they took before.
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* A server rejection that names fields must mark those fields.
11+
*
12+
* `@objectstack/objectql`'s validators throw `VALIDATION_FAILED` with
13+
* `fields[]`, and both the REST layer and the runtime dispatcher serve it as a
14+
* 400 with those entries intact. The form caught the rejection, showed one
15+
* generic toast, and dropped `fields[]` — so a user told "Validation failed"
16+
* had to guess WHICH input was wrong, on a surface that already knows how to
17+
* mark it and already does exactly that for client-side validation.
18+
*
19+
* The sibling test `form-invalid-scroll.test.tsx` pins the client-side half;
20+
* these two paths now share one implementation, so they must behave alike.
21+
*/
22+
23+
import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest';
24+
import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react';
25+
import { ComponentRegistry } from '@object-ui/core';
26+
import { toast } from '../../../ui/sonner';
27+
28+
beforeAll(async () => {
29+
await import('../../../renderers');
30+
}, 30000);
31+
32+
let toastErrorSpy: ReturnType<typeof vi.spyOn>;
33+
34+
beforeEach(() => {
35+
toastErrorSpy = vi.spyOn(toast, 'error').mockImplementation(() => 'id' as any);
36+
if (!(Element.prototype as any).scrollIntoView) {
37+
(Element.prototype as any).scrollIntoView = () => {};
38+
}
39+
});
40+
41+
afterEach(() => {
42+
cleanup();
43+
vi.restoreAllMocks();
44+
});
45+
46+
/** The wire shape the server sends, as `@objectstack/client` decorates it. */
47+
function validationFailure(fields: Array<{ field: string; code?: string; message?: string }>) {
48+
const message = fields.map((f) => f.message ?? `${f.field} (${f.code})`).join('; ');
49+
return Object.assign(new Error(message), {
50+
code: 'VALIDATION_FAILED',
51+
httpStatus: 400,
52+
details: { error: message, code: 'VALIDATION_FAILED', fields, object: 'crm_opportunity' },
53+
});
54+
}
55+
56+
function renderForm(fields: any[], onSubmit: (data: any) => Promise<unknown>) {
57+
const Form = ComponentRegistry.get('form')!;
58+
return render(
59+
<Form
60+
schema={{
61+
type: 'form',
62+
mode: 'create',
63+
showSubmit: true,
64+
showCancel: false,
65+
submitLabel: 'Create',
66+
fields,
67+
onSubmit,
68+
}}
69+
/>,
70+
);
71+
}
72+
73+
const submit = () => fireEvent.click(screen.getByRole('button', { name: /create/i }));
74+
75+
const FIELDS = [
76+
{ name: 'title', label: 'Title', type: 'input' },
77+
{ name: 'stage', label: 'Stage', type: 'input' },
78+
];
79+
80+
describe('form renderer — server-rejected fields', () => {
81+
it("writes the server's reason under the offending input", async () => {
82+
const onSubmit = vi.fn().mockRejectedValue(
83+
validationFailure([{ field: 'title', code: 'required', message: 'Title is required' }]),
84+
);
85+
renderForm(FIELDS, onSubmit);
86+
87+
submit();
88+
89+
await waitFor(() => expect(screen.getByText('Title is required')).toBeTruthy());
90+
});
91+
92+
it('scrolls and focuses the first rejected field, as a client-side failure does', async () => {
93+
const onSubmit = vi.fn().mockRejectedValue(
94+
validationFailure([{ field: 'stage', code: 'invalid_option', message: 'Stage is not a valid option' }]),
95+
);
96+
const { container } = renderForm(FIELDS, onSubmit);
97+
const stageWrap = container.querySelector('[data-field="stage"]') as HTMLElement;
98+
const scrollSpy = vi.fn();
99+
stageWrap.scrollIntoView = scrollSpy;
100+
101+
submit();
102+
103+
await waitFor(() => expect(scrollSpy).toHaveBeenCalled());
104+
await waitFor(() => expect(stageWrap.contains(document.activeElement)).toBe(true));
105+
});
106+
107+
it('names the rejected fields in the toast', async () => {
108+
const onSubmit = vi.fn().mockRejectedValue(
109+
validationFailure([{ field: 'title', message: 'Title is required' }]),
110+
);
111+
renderForm(FIELDS, onSubmit);
112+
113+
submit();
114+
115+
await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled());
116+
// The field LABEL, not its machine name.
117+
expect(String(toastErrorSpy.mock.calls[0][0])).toMatch(/Title/);
118+
});
119+
120+
it('marks every rejected field, not just the first', async () => {
121+
const onSubmit = vi.fn().mockRejectedValue(
122+
validationFailure([
123+
{ field: 'title', message: 'Title is required' },
124+
{ field: 'stage', message: 'Stage is not a valid option' },
125+
]),
126+
);
127+
renderForm(FIELDS, onSubmit);
128+
129+
submit();
130+
131+
await waitFor(() => expect(screen.getByText('Title is required')).toBeTruthy());
132+
expect(screen.getByText('Stage is not a valid option')).toBeTruthy();
133+
});
134+
135+
it('falls back to the generic banner when a rejected field has no input to carry it', async () => {
136+
// A field the form does not render cannot be marked inline. Taking over the
137+
// failure anyway would silently drop the part the user cannot see, so the
138+
// top-level message — which names every field — is shown instead.
139+
const onSubmit = vi.fn().mockRejectedValue(
140+
validationFailure([
141+
{ field: 'title', message: 'Title is required' },
142+
{ field: 'owner_id', message: 'Owner is required' },
143+
]),
144+
);
145+
renderForm(FIELDS, onSubmit);
146+
147+
submit();
148+
149+
await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled());
150+
expect(String(toastErrorSpy.mock.calls[0][0])).toMatch(/Owner is required/);
151+
});
152+
153+
it('leaves a non-field failure on the generic path', async () => {
154+
const forbidden = Object.assign(new Error('insufficient privileges'), {
155+
code: 'FORBIDDEN',
156+
httpStatus: 403,
157+
details: { error: 'insufficient privileges', code: 'FORBIDDEN' },
158+
});
159+
const onSubmit = vi.fn().mockRejectedValue(forbidden);
160+
const { container } = renderForm(FIELDS, onSubmit);
161+
const titleWrap = container.querySelector('[data-field="title"]') as HTMLElement;
162+
const scrollSpy = vi.fn();
163+
titleWrap.scrollIntoView = scrollSpy;
164+
165+
submit();
166+
167+
await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled());
168+
// Nothing is attributable to a field, so no input is marked or scrolled to.
169+
expect(scrollSpy).not.toHaveBeenCalled();
170+
});
171+
});

packages/components/src/renderers/form/form.tsx

Lines changed: 72 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import { AlertCircle, ChevronDown, ChevronRight, Loader2, Maximize2, Check, X }
2929
import { Dialog, DialogContent, DialogDescription, DialogHeader, DialogTitle, DialogFooter } from '../../ui/dialog';
3030
import { cn } from '../../lib/utils';
3131
import React from 'react';
32-
import { SchemaRendererContext, usePredicateScope, isPermissionError, extractWriteErrorMessage } from '@object-ui/react';
32+
import { SchemaRendererContext, usePredicateScope, isPermissionError, extractWriteErrorMessage, extractFieldErrors } from '@object-ui/react';
3333
import { createSafeTranslation } from '@object-ui/i18n';
3434

3535
/** Inline section header rendered as a virtual field inside a flat SchemaRenderer field list.
@@ -502,6 +502,45 @@ ComponentRegistry.register('form',
502502
return () => subscription.unsubscribe();
503503
}, [form, onDirtyChangeProp]);
504504

505+
/**
506+
* Tell the user WHICH fields are wrong, wherever the verdict came from.
507+
*
508+
* Toast naming the offending fields, then scroll the FIRST of them into
509+
* view (in declared/visual order, not the caller's key order) and focus a
510+
* control inside it — the field wrapper carries `data-field` (FormItem), so
511+
* this reaches custom widgets that RHF's own focus cannot (#2793).
512+
*
513+
* Extracted from the client-side invalid handler so the SERVER-rejection
514+
* path gets identical treatment. The two failures are the same event as far
515+
* as the person filling the form is concerned; only the referee differs.
516+
*/
517+
const announceFieldErrors = (names: string[]) => {
518+
if (names.length === 0) return;
519+
const labels = names.map((n) => fieldLabelByName[n] || n);
520+
const MAX = 3;
521+
const fieldsText = labels.slice(0, MAX).join('、') + (labels.length > MAX ? '…' : '');
522+
toast.error(t('validation.formInvalid', { fields: fieldsText }));
523+
524+
const errored = new Set(names);
525+
const firstName =
526+
(fields as FormFieldConfig[])
527+
.map((f) => f?.name)
528+
.find((n): n is string => Boolean(n) && errored.has(n as string)) ?? names[0];
529+
const root: ParentNode = formRef.current ?? document;
530+
const escaped =
531+
typeof CSS !== 'undefined' && typeof CSS.escape === 'function'
532+
? CSS.escape(firstName)
533+
: firstName.replace(/["\\]/g, '\\$&');
534+
const target = root.querySelector<HTMLElement>(`[data-field="${escaped}"]`);
535+
if (target) {
536+
target.scrollIntoView?.({ behavior: 'smooth', block: 'center' });
537+
const focusable = target.querySelector<HTMLElement>(
538+
'input:not([type="hidden"]), select, textarea, button, [tabindex]:not([tabindex="-1"]), [contenteditable="true"]',
539+
);
540+
focusable?.focus?.({ preventScroll: true });
541+
}
542+
};
543+
505544
// Handle form submission
506545
const handleSubmit = form.handleSubmit(async (data) => {
507546
setIsSubmitting(true);
@@ -555,6 +594,37 @@ ComponentRegistry.register('form',
555594
form.reset();
556595
}
557596
} catch (error) {
597+
// The server can reject field-by-field: `@objectstack/objectql`'s
598+
// validators throw `VALIDATION_FAILED` with `fields[]`, and both the
599+
// REST layer and the runtime dispatcher serve it as a 400 with those
600+
// entries intact. Every form used to drop them and show one undirected
601+
// toast — telling the user something was wrong, but not WHAT, on a
602+
// surface that already knows how to mark the input. Mark them.
603+
const fieldErrors = extractFieldErrors(error);
604+
if (fieldErrors) {
605+
const known = fieldErrors.filter((fe) =>
606+
(fields as FormFieldConfig[]).some((f) => f?.name === fe.field),
607+
);
608+
for (const fe of known) {
609+
form.setError(fe.field, {
610+
type: 'server',
611+
// Fall back to the envelope's top-level text rather than leaving
612+
// a marked field with no reason next to it.
613+
message: fe.message || extractWriteErrorMessage(error) || t('form.submitFailed'),
614+
});
615+
}
616+
// Only take over the whole failure when EVERY rejected field has a
617+
// visible input to carry it. If the server also rejected something
618+
// this form does not render, fall through: the top-level message
619+
// concatenates every field's reason, so the part the user cannot see
620+
// inline is still said out loud instead of silently dropped.
621+
if (known.length > 0 && known.length === fieldErrors.length) {
622+
announceFieldErrors(known.map((fe) => fe.field));
623+
// A fully-attributed rejection is not a form-level failure — the
624+
// banner would just repeat what is now written under each input.
625+
return;
626+
}
627+
}
558628
// A rejected write used to be rendered verbatim, which put raw server
559629
// diagnostics in front of end users — untranslated, and leaking the
560630
// object's machine name and the record id, e.g.
@@ -587,36 +657,7 @@ ComponentRegistry.register('form',
587657
// is often scrolled out of view — the user clicks 创建 and sees nothing
588658
// happen. Surface a toast naming the fields so the feedback is visible
589659
// regardless of scroll position (mirrors the server-error toast above).
590-
const names = Object.keys(validationErrors || {});
591-
if (names.length === 0) return;
592-
const labels = names.map((n) => fieldLabelByName[n] || n);
593-
const MAX = 3;
594-
const fieldsText = labels.slice(0, MAX).join('、') + (labels.length > MAX ? '…' : '');
595-
toast.error(t('validation.formInvalid', { fields: fieldsText }));
596-
597-
// #2793: the toast alone still leaves the user hunting — in a long form
598-
// the offending field is off-screen. Scroll the FIRST errored field into
599-
// view (in declared/visual order, not RHF's error-key order) and focus a
600-
// focusable control inside it. The field wrapper carries `data-field`
601-
// (FormItem); works for custom widgets that RHF's own focus can't reach.
602-
const errored = new Set(names);
603-
const firstName =
604-
(fields as FormFieldConfig[])
605-
.map((f) => f?.name)
606-
.find((n): n is string => Boolean(n) && errored.has(n as string)) ?? names[0];
607-
const root: ParentNode = formRef.current ?? document;
608-
const escaped =
609-
typeof CSS !== 'undefined' && typeof CSS.escape === 'function'
610-
? CSS.escape(firstName)
611-
: firstName.replace(/["\\]/g, '\\$&');
612-
const target = root.querySelector<HTMLElement>(`[data-field="${escaped}"]`);
613-
if (target) {
614-
target.scrollIntoView?.({ behavior: 'smooth', block: 'center' });
615-
const focusable = target.querySelector<HTMLElement>(
616-
'input:not([type="hidden"]), select, textarea, button, [tabindex]:not([tabindex="-1"]), [contenteditable="true"]',
617-
);
618-
focusable?.focus?.({ preventScroll: true });
619-
}
660+
announceFieldErrors(Object.keys(validationErrors || {}));
620661
});
621662

622663
// Handle cancel

0 commit comments

Comments
 (0)