diff --git a/.changeset/27520-refused-grid-and-form-saves.md b/.changeset/27520-refused-grid-and-form-saves.md new file mode 100644 index 00000000000..98c478357e2 --- /dev/null +++ b/.changeset/27520-refused-grid-and-form-saves.md @@ -0,0 +1,14 @@ +--- +'@object-ui/components': patch +'@object-ui/plugin-grid': patch +--- + +fix(ui): a refused inline-grid save marks its cells and says every reason; a refused form save is said once + +Clause-②: no +Metadata-protocol: none + +- **Inline grid edit, Save All.** When the server refused a row, the save bar showed one shortened line, with the rest only in a tooltip, and no cell showed which value was wrong. Now every reason is shown in full on its own line under the toolbar, and each cell the refusal names gets a red outline with its reason on hover. Editing a marked cell removes its outline. +- **Field rules before sending.** The grid's built-in saves now check an edited value against the field's own rules (required, min / max, length, email / URL pattern) before sending, and refuse a row that breaks one in the same way the server would. That row is not sent; the other rows still save. +- **Save All placement.** The grid toolbar is padded, so the Save All / Cancel All buttons no longer sit against the right edge of the grid. +- **Refused form save.** When the server refused a form save, such as one blocked by a validation rule on a Studio-authored object, the form showed a banner and an identical error toast. It now shows the banner only, scrolled into view, and a retry still replaces that banner instead of adding another. This reverses the earlier one-error-toast behaviour. diff --git a/ui/packages/components/src/__tests__/data-table-refusal-cells-27520.test.tsx b/ui/packages/components/src/__tests__/data-table-refusal-cells-27520.test.tsx new file mode 100644 index 00000000000..937a3bd1a20 --- /dev/null +++ b/ui/packages/components/src/__tests__/data-table-refusal-cells-27520.test.tsx @@ -0,0 +1,160 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectstack-ai/objectstack#27520 (row 6) — a refused inline save says WHY, + * WHERE: the save bar says every reason whole, one line each, and the cells + * the refusal names are marked. + * + * The server refuses a row field by field (`VALIDATION_FAILED` with one + * `fields[]` entry per failed rule). The save bar used to cut that to one + * truncated line, the rest only in a native tooltip, and no cell said which + * value was wrong. + */ +import { describe, it, expect, vi } from 'vitest'; +import { fireEvent, waitFor, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { renderComponent } from './test-utils'; +// Registers the renderers at module scope, NOT inside a `beforeAll`; see +// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). +import '../renderers'; + +const schema = { + type: 'data-table' as const, + editable: true, + singleClickEdit: true, + columns: [ + { header: 'Title', accessorKey: 'title' }, + { header: 'Estimate', accessorKey: 'estimate', type: 'number' }, + ], + data: [ + { id: '1', title: 'Ingest pipeline', estimate: 60 }, + { id: '2', title: 'Warehouse schema', estimate: 33 }, + ], +} as any; + +type Entry = { rowIndex: number; changes: Record; row: any }; + +const TITLE = 'Title is required and cannot be cleared'; +const ESTIMATE = 'Estimate (h) must be ≥ 0'; + +/** The refusal `PATCH /data/:object/:id` answers, as `@objectstack/client` decorates it. */ +function fieldRefusal(fields: Array<{ field: string; message: string }>) { + const message = fields.map((f) => f.message).join('; '); + return Object.assign(new Error(message), { + code: 'VALIDATION_FAILED', + httpStatus: 400, + details: { error: message, code: 'VALIDATION_FAILED', fields: fields.map((f) => ({ ...f, code: 'invalid' })) }, + }); +} + +const cellOf = (container: HTMLElement, rowNumber: number, colNumber: number) => + container.querySelectorAll('tbody tr')[rowNumber].querySelectorAll('td')[colNumber] as HTMLElement; + +/** Stage a new value in a body cell. */ +function stage(container: HTMLElement, rowNumber: number, colNumber: number, value: string) { + const cell = cellOf(container, rowNumber, colNumber); + fireEvent.click(cell); + const input = cell.querySelector('input') as HTMLInputElement; + fireEvent.change(input, { target: { value } }); + fireEvent.blur(input); +} + +const refusalBar = (container: HTMLElement) => + container.querySelector('[data-testid="table-save-refusal"]') as HTMLElement | null; + +describe('data-table — a refused save says every reason and marks its cells (#27520)', () => { + it('Save All: one line per reason, whole, and the two named cells of the refused row marked', async () => { + const onBatchSave = vi.fn(async (entries: Entry[]) => ({ + failed: entries + .filter((e) => e.row.id === '1') + .map((e) => ({ + rowIndex: e.rowIndex, + error: fieldRefusal([{ field: 'title', message: TITLE }, { field: 'estimate', message: ESTIMATE }]), + })), + })); + const { container, getByText } = renderComponent({ ...schema, onBatchSave }); + + stage(container, 0, 0, ''); + stage(container, 0, 1, '-5'); + stage(container, 1, 1, '31'); + fireEvent.click(getByText(/Save All/)); + + await waitFor(() => expect(refusalBar(container)).not.toBeNull()); + const bar = refusalBar(container)!; + expect(bar).toHaveAttribute('role', 'alert'); + expect(within(bar).getAllByRole('listitem').map((li) => li.textContent)).toEqual([TITLE, ESTIMATE]); + // Said whole: not the `; `-joined summary, and not cut. + expect(bar.textContent).not.toContain('; '); + expect(bar.querySelector('.truncate')).toBeNull(); + + // The refused row's two cells carry their reasons; the saved row's none. + expect(cellOf(container, 0, 0)).toHaveAttribute('aria-invalid', 'true'); + expect(cellOf(container, 0, 0)).toHaveAttribute('data-cell-error', TITLE); + expect(cellOf(container, 0, 1)).toHaveAttribute('aria-invalid', 'true'); + expect(cellOf(container, 0, 1)).toHaveAttribute('data-cell-error', ESTIMATE); + expect(container.querySelectorAll('tbody tr')[1].querySelector('[aria-invalid="true"]')).toBeNull(); + }); + + it('editing a marked cell takes its mark away and leaves the other', async () => { + const onBatchSave = vi.fn(async (entries: Entry[]) => ({ + failed: entries.map((e) => ({ + rowIndex: e.rowIndex, + error: fieldRefusal([{ field: 'title', message: TITLE }, { field: 'estimate', message: ESTIMATE }]), + })), + })); + const { container, getByText } = renderComponent({ ...schema, onBatchSave }); + + stage(container, 0, 0, ''); + stage(container, 0, 1, '-5'); + fireEvent.click(getByText(/Save All/)); + await waitFor(() => expect(cellOf(container, 0, 1)).toHaveAttribute('aria-invalid', 'true')); + + stage(container, 0, 1, '5'); + + await waitFor(() => expect(cellOf(container, 0, 1)).not.toHaveAttribute('aria-invalid')); + expect(cellOf(container, 0, 0)).toHaveAttribute('aria-invalid', 'true'); + }); + + it('a single-row save marks the cell its refusal names, and a later save that lands clears it', async () => { + const onRowSave = vi + .fn() + .mockRejectedValueOnce(fieldRefusal([{ field: 'estimate', message: ESTIMATE }])) + .mockResolvedValueOnce(undefined); + // `rowActions` draws the edit column that holds the row's own save button. + const { container } = renderComponent({ ...schema, rowActions: true, onRowSave }); + + stage(container, 0, 1, '-5'); + // The row's own save button (the edit column's save icon). + const saveRow = () => + fireEvent.click(within(container.querySelectorAll('tbody tr')[0] as HTMLElement).getByTitle(/^Save row$/i)); + saveRow(); + + await waitFor(() => expect(cellOf(container, 0, 1)).toHaveAttribute('data-cell-error', ESTIMATE)); + expect(refusalBar(container)?.textContent).toContain(ESTIMATE); + + stage(container, 0, 1, '5'); + saveRow(); + await waitFor(() => expect(onRowSave).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(container.querySelector('tbody [aria-invalid="true"]')).toBeNull()); + }); + + it('a whole-batch rejection says its reasons but marks no cell: no row is named', async () => { + const onBatchSave = vi.fn(async () => { + throw fieldRefusal([{ field: 'title', message: TITLE }]); + }); + const { container, getByText } = renderComponent({ ...schema, onBatchSave }); + + stage(container, 0, 0, ''); + stage(container, 1, 1, '31'); + fireEvent.click(getByText(/Save All/)); + + await waitFor(() => expect(refusalBar(container)?.textContent).toContain(TITLE)); + expect(container.querySelector('tbody [aria-invalid="true"]')).toBeNull(); + }); +}); diff --git a/ui/packages/components/src/renderers/complex/data-table.tsx b/ui/packages/components/src/renderers/complex/data-table.tsx index cd30e7a42f7..8c1517bd5e8 100644 --- a/ui/packages/components/src/renderers/complex/data-table.tsx +++ b/ui/packages/components/src/renderers/complex/data-table.tsx @@ -18,7 +18,7 @@ import { describeIgnoredBind, describeNonArrayData } from './dataTableBindDiagno import { ComponentRegistry, compareSortValues, evalRowPredicate, formatDate, hasDeclaredPredicate, isActionSwitchedOff, formatDateTime, fromDateTimeInputValue, getSortValue, isEmptyValue, isImpossibleStoredDay, toDateInputValue, toDateTimeInputValue } from '@object-ui/core'; import type { DataTableSchema, TableColumn, TableSortItem, TableColumnType } from '@object-ui/types'; import type { SortDirection } from '@objectstack/spec/shared'; -import { SchemaRenderer, toRenderableSchema, useCapabilityGate, useRowPredicate, usePredicateScope } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema, useCapabilityGate, useRowPredicate, usePredicateScope, extractFieldErrors, unmarkedRefusalLines } from '@object-ui/react'; import { createSafeTranslation, useDisplayLocale } from '@object-ui/i18n'; import { Table, @@ -374,6 +374,39 @@ function extractSaveErrorMessage(error: unknown): string { return typeof error === 'string' && error.trim() ? error.trim() : 'Unknown error'; } +/** + * The lines a refused save shows in the save bar: one per reason the refusals + * list (a `VALIDATION_FAILED` answer keeps one `fields[]` entry per failed + * rule), else each refusal's one message — distinct, in the order the rows + * were sent. Said whole, never cut to one line (objectstack-ai/objectstack#27520). + */ +function saveRefusalLines(errors: readonly unknown[]): string[] { + const lines: string[] = []; + for (const error of errors) { + const listed = unmarkedRefusalLines(error, () => false); + for (const line of listed && listed.length > 0 ? listed : [extractSaveErrorMessage(error)]) { + if (!lines.includes(line)) lines.push(line); + } + } + return lines; +} + +/** + * The cells a refused row save names, field → reason, read from the refusal's + * own field entries (`extractFieldErrors`); `null` when it names none. A field + * the table draws no column for simply marks nothing: its reason is still in + * the save bar. + */ +function refusedCellsOf(error: unknown): Record | null { + const entries = extractFieldErrors(error); + if (!entries) return null; + const cells: Record = {}; + for (const { field, message } of entries) { + if (!(field in cells)) cells[field] = message || extractSaveErrorMessage(error); + } + return cells; +} + /** * The element type of `DataTableSchema.rowActionDefs`. Derived via indexed * access rather than imported by name: `@object-ui/types` defines @@ -1670,9 +1703,35 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { }; }, [pendingChanges]); const [isSaving, setIsSaving] = useState(false); - // Last save failure message (server validation text, etc.) shown in the - // toolbar; null when the last save attempt succeeded or nothing's been saved. - const [saveError, setSaveError] = useState(null); + // Last save failure, one line per reason (server validation text, etc.), + // shown in the toolbar; null when the last save attempt succeeded or + // nothing's been saved. + const [saveError, setSaveError] = useState(null); + // The cells the last refusal named, per record key, field → reason + // (objectstack-ai/objectstack#27520): each is marked where it is drawn, and + // its mark goes the moment the cell is edited again or its row saves. + const [cellErrors, setCellErrors] = useState>>(new Map()); + const clearCellError = (rowKey: PendingRowKey, columnKey: string) => { + setCellErrors((prev) => { + const cells = prev.get(rowKey); + if (!cells || !(columnKey in cells)) return prev; + const next = new Map(prev); + const rest = { ...cells }; + delete rest[columnKey]; + if (Object.keys(rest).length > 0) next.set(rowKey, rest); + else next.delete(rowKey); + return next; + }); + }; + const setRowCellErrors = (rowKey: PendingRowKey, cells: Record | null) => { + setCellErrors((prev) => { + if (!cells && !prev.has(rowKey)) return prev; + const next = new Map(prev); + if (cells) next.set(rowKey, cells); + else next.delete(rowKey); + return next; + }); + }; // Records whose last save attempt failed, by the key their pending edit is // kept under (objectui#12310) — tinted destructive so the author sees exactly // which rows didn't persist (no silent "phantom save"). @@ -2335,6 +2394,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // save that settled since this render is not undone (objectui#12322). const inFlight = inFlightRef.current.get(rowKey); setPendingChanges((prev) => stageCellChange(prev, rowKey, row, columnKey, valueToStage, inFlight)); + clearCellError(rowKey, columnKey); // Call the legacy onCellChange callback if provided if (schema.onCellChange) { @@ -2428,6 +2488,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { const inFlight = inFlightRef.current.get(rowKey); setEditValue(value); setPendingChanges((prev) => stageCellChange(prev, rowKey, row, columnKey, value, inFlight)); + clearCellError(rowKey, columnKey); }; // Commit the in-flight edit when the input loses focus (e.g. the user clicks @@ -2502,12 +2563,15 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { if (next.size === 0) setSaveError(null); return next; }); + setRowCellErrors(rowKey, null); } catch (error) { // Keep the pending change so the author can fix and retry; surface the - // reason instead of failing silently, and flag the row. + // reason instead of failing silently, flag the row, and mark the cells + // the refusal names. console.error('Failed to save row:', error); - setSaveError(extractSaveErrorMessage(error)); + setSaveError(saveRefusalLines([error])); setErroredRows((prev) => new Set(prev).add(rowKey)); + setRowCellErrors(rowKey, refusedCellsOf(error)); } finally { setIsSaving(false); } @@ -2524,6 +2588,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { if (next.size === 0) setSaveError(null); return next; }); + setRowCellErrors(rowKey, null); }; // Save All. Like `saveRow`, it settles against the pending edits as they @@ -2589,13 +2654,23 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { const landed = new Map(Array.from(sent).filter(([key]) => !failedKeys.has(key))); setPendingChanges((prev) => removeSentEdits(prev, landed)); setErroredRows(failedKeys); - setSaveError(extractSaveErrorMessage(failed[0].error)); + // Every refused row's reasons, and the cells each refusal names + // (objectstack-ai/objectstack#27520). + setSaveError(saveRefusalLines(failed.map((entry) => entry.error))); + const refusedCells = new Map>(); + for (const entry of failed) { + const cells = refusedCellsOf(entry.error); + const key = keyOfRowIndex.get(entry.rowIndex); + if (cells && key !== undefined) refusedCells.set(key, cells); + } + setCellErrors(refusedCells); return; } // Clear every edit the batch saved, and only those. setPendingChanges((prev) => removeSentEdits(prev, sent)); // Saved — clear any prior errors. setErroredRows(new Set()); + setCellErrors(new Map()); setSaveError(null); } catch (error) { // A rejection says nothing about which rows landed, so it is read as @@ -2603,8 +2678,11 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // one staged during the save), and surface the reason instead of // failing silently. console.error('Failed to save batch:', error); - setSaveError(extractSaveErrorMessage(error)); + setSaveError(saveRefusalLines([error])); setErroredRows(new Set(sent.keys())); + // Which row a whole-batch refusal's fields belong to is unknown, so no + // cell is marked; the save bar says every reason. + setCellErrors(new Map()); } finally { setIsSaving(false); } @@ -2613,6 +2691,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { const cancelAllChanges = () => { setPendingChanges(new Map()); setErroredRows(new Set()); + setCellErrors(new Map()); setSaveError(null); }; @@ -3022,7 +3101,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
{/* Toolbar */} {showToolbar && ( -
+
{searchEnabled && (
@@ -3040,17 +3119,6 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
{hasPendingChanges && ( <> - {saveError && ( -
- - - {t('table.saveFailed')}: {saveError} - -
- )}
{t('table.modified', { count: pendingChanges.size })}
@@ -3093,6 +3161,31 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
)}
+ + {/* The last save's refusal, on a line of its own and said whole — + one line per reason (objectstack-ai/objectstack#27520). The + cells it names are marked in the table below. */} + {hasPendingChanges && saveError && ( +
+ + {saveError.length === 1 ? ( + + {t('table.saveFailed')}: {saveError[0]} + + ) : ( +
+ {t('table.saveFailed')}: +
    + {saveError.map((line) =>
  • {line}
    • )} +
+
+ )} +
+ )}
)} @@ -3453,6 +3546,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { const rowHasChanges = rowPending !== undefined; const rowChanges = rowPending?.changes || {}; const rowErrored = erroredRows.has(rowKey); + const rowCellErrors = cellErrors.get(rowKey); return ( // objectstack#27545 — the row of the record a pane beside @@ -3669,6 +3763,9 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { const hasPendingChange = rowChanges[col.accessorKey] !== undefined; const cellValue = hasPendingChange ? rowChanges[col.accessorKey] : originalValue; const isEditing = editingCell !== null && editingCell.rowKey === rowKey && editingCell.columnKey === col.accessorKey; + // The reason the last refusal gave for THIS cell, if it + // named it (objectstack-ai/objectstack#27520). + const cellError = rowCellErrors?.[col.accessorKey]; // A masked column reads as NOT editable here too // (objectui#10583), so its cell neither shows the // edit cursor nor swallows the row's click — @@ -3700,6 +3797,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { isFit ? 'whitespace-nowrap' : 'overflow-hidden', isEditable && !isEditing && "cursor-text hover:bg-muted/50", hasPendingChange && "font-semibold text-amber-700 dark:text-amber-400", + cellError !== undefined && "ring-2 ring-inset ring-destructive", isFrozen && ['sticky z-10 bg-background', STICKY_ROW_TINT], isFrozen && colIndex === frozenColumns - 1 && 'border-r-2 border-border shadow-[2px_0_4px_-2px_rgba(0,0,0,0.1)]', )} @@ -3742,6 +3840,9 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { tabIndex={keyboardNavigation ? (rowIndex === rovingRowIndex && colIndex === rovingColIndex ? 0 : -1) : 0} data-grid-cell={keyboardNavigation ? `${rowIndex}:${colIndex}` : undefined} onFocus={keyboardNavigation ? () => rove(rowIndex, colIndex) : undefined} + aria-invalid={cellError !== undefined || undefined} + data-cell-error={cellError} + title={cellError} > {isEditing ? ( (() => { diff --git a/ui/packages/components/src/renderers/form/__tests__/fieldtab-visiblewhen-6237.test.tsx b/ui/packages/components/src/renderers/form/__tests__/fieldtab-visiblewhen-6237.test.tsx index ee725eac242..20ec9c236e0 100644 --- a/ui/packages/components/src/renderers/form/__tests__/fieldtab-visiblewhen-6237.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/fieldtab-visiblewhen-6237.test.tsx @@ -238,8 +238,9 @@ describe('#6237 — a tab predicate gates the whole tab', () => { fireEvent.click(submitBtn()); await waitFor(() => expect(screen.getByText(message)).toBeTruthy()); - expect(toastError).toHaveBeenCalledTimes(1); - expect(String(toastError.mock.calls[0][0])).toBe(message); + // Said once, by the banner (objectstack-ai/objectstack#27520). + expect(screen.getByRole('alert').textContent).toBe(message); + expect(toastError).not.toHaveBeenCalled(); // The hidden tab is never activated to "reveal" a field it cannot draw. expect(trigger()).toBeNull(); expect(screen.getByRole('tab', { name: /basics/i })).toHaveAttribute('data-state', 'active'); diff --git a/ui/packages/components/src/renderers/form/__tests__/form-invalid-toast-joiner.test.tsx b/ui/packages/components/src/renderers/form/__tests__/form-invalid-toast-joiner.test.tsx index 7be6e840689..919c6418bd4 100644 --- a/ui/packages/components/src/renderers/form/__tests__/form-invalid-toast-joiner.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/form-invalid-toast-joiner.test.tsx @@ -34,7 +34,7 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; import { ComponentRegistry } from '@object-ui/core'; import { I18nProvider } from '@object-ui/i18n'; import { toast } from '../../../ui/sonner'; @@ -87,11 +87,16 @@ function renderFormIn(language: string) { ); } +/** + * The refusal as the form says it: its banner, the one message a refused + * submit raises (objectstack-ai/objectstack#27520 retired the toast copy). + */ async function toastTextAfterSubmit(language: string): Promise { renderFormIn(language); fireEvent.click(screen.getByRole('button', { name: /create/i })); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled()); - return String(toastErrorSpy.mock.calls[0][0]); + const banner = await screen.findByRole('alert'); + expect(toastErrorSpy).not.toHaveBeenCalled(); + return banner.textContent ?? ''; } describe('form renderer — refused-submit field list joiner (objectstack#5407)', () => { diff --git a/ui/packages/components/src/renderers/form/__tests__/form-one-toast-per-refusal-13340.test.tsx b/ui/packages/components/src/renderers/form/__tests__/form-one-toast-per-refusal-13340.test.tsx index d41e026a92a..e49c3c3ade8 100644 --- a/ui/packages/components/src/renderers/form/__tests__/form-one-toast-per-refusal-13340.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/form-one-toast-per-refusal-13340.test.tsx @@ -7,18 +7,17 @@ */ /** - * A refused save raises ONE error toast (objectui#13340). + * A refused save is said ONCE (objectui#13340, objectstack-ai/objectstack#27520). * * The console's record form (`ModalForm`, `RecordFormPage`'s `ObjectForm`) * hands this renderer an `onSubmit` that writes through the data source and, * on a refusal, rethrows it. This renderer is the one that reports it: the - * banner above the fields, plus a toast for when the banner has scrolled out - * of view. The toast is raised under the form's own outcome id, so a retry - * that is refused again REPLACES the previous toast instead of stacking a - * second copy beside it. + * banner above the fields, brought into view each time it is raised, and no + * error toast repeating it. A retry that is refused again keeps that one + * banner rather than stacking a second message beside it. * * Pinned on the toast calls themselves (every `toast.error` this form makes, - * undeduplicated), not on what sonner ends up showing, so a second raiser here + * undeduplicated), not on what sonner ends up showing, so a raiser here * cannot hide behind sonner's id replacement. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -33,12 +32,14 @@ import '../../../renderers'; const REFUSAL = 'A record with this value already exists'; let toastErrorSpy: ReturnType; +let scrolled: Element[]; beforeEach(() => { toastErrorSpy = vi.spyOn(toast, 'error').mockImplementation(() => 'id' as any); - if (!(Element.prototype as any).scrollIntoView) { - (Element.prototype as any).scrollIntoView = () => {}; - } + scrolled = []; + (Element.prototype as any).scrollIntoView = function (this: Element) { + scrolled.push(this); + }; }); afterEach(() => { @@ -70,30 +71,33 @@ function renderRefusingForm() { const create = () => fireEvent.click(screen.getByRole('button', { name: /create/i })); -describe('form renderer — one refusal, one error toast (objectui#13340)', () => { - it('a refused submit raises exactly one error toast, with the server`s sentence', async () => { +describe('form renderer — one refusal, one message (objectui#13340, objectstack-ai/objectstack#27520)', () => { + it('a refused submit draws one banner, scrolled into view, and raises no error toast', async () => { renderRefusingForm(); create(); await waitFor(() => expect(refuse).toHaveBeenCalledTimes(1)); - // The banner is the settle signal: it is set in the same catch as the toast. - await screen.findByText(REFUSAL); - expect(toastErrorSpy).toHaveBeenCalledTimes(1); - expect(toastErrorSpy.mock.calls[0][0]).toBe(REFUSAL); + // The banner is the settle signal: it is set in the same catch. + const banner = (await screen.findByText(REFUSAL)).closest('[role="alert"]'); + expect(banner).not.toBeNull(); + expect(screen.getAllByRole('alert')).toHaveLength(1); + await waitFor(() => expect(scrolled).toContain(banner)); + expect(toastErrorSpy).not.toHaveBeenCalled(); }); - it('a refused retry raises one more, under the same id, so it replaces the first', async () => { + it('a refused retry keeps one banner, brings it into view again, and still raises no toast', async () => { refuse.mockClear(); renderRefusingForm(); create(); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalledTimes(1)); + await screen.findByText(REFUSAL); + await waitFor(() => expect(scrolled.length).toBe(1)); create(); await waitFor(() => expect(refuse).toHaveBeenCalledTimes(2)); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(scrolled.length).toBe(2)); - const [first, second] = toastErrorSpy.mock.calls.map((c: unknown[]) => (c[1] as { id?: unknown } | undefined)?.id); - expect(first).toBeDefined(); - expect(second).toBe(first); + expect(screen.getAllByText(REFUSAL)).toHaveLength(1); + expect(screen.getAllByRole('alert')).toHaveLength(1); + expect(toastErrorSpy).not.toHaveBeenCalled(); }); }); diff --git a/ui/packages/components/src/renderers/form/__tests__/form-refusal-lines-28053.test.tsx b/ui/packages/components/src/renderers/form/__tests__/form-refusal-lines-28053.test.tsx index 2ee22702d8e..90938502e22 100644 --- a/ui/packages/components/src/renderers/form/__tests__/form-refusal-lines-28053.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/form-refusal-lines-28053.test.tsx @@ -80,20 +80,13 @@ function bannerLines(container: HTMLElement): string[] { return items.length > 0 ? items.map((li) => li.textContent ?? '') : [banner.textContent ?? '']; } -/** The toast's content, drawn, as text lines. */ -function toastLines(): string[] { - const content = toastErrorSpy.mock.calls.at(-1)![0] as React.ReactNode; - if (typeof content === 'string') return [content]; - const { container } = render(<>{content}); - return Array.from(container.querySelectorAll('li')).map((li) => li.textContent ?? ''); -} const AMOUNT = 'Amount must be positive.'; const NAME = 'Name is required for a forecast.'; const BEST = 'Best case must be at least the amount.'; describe('form renderer — one entry per failed validation rule (#28053)', () => { - it('two record-level rules: two banner lines and two toast lines, no joined summary', async () => { + it('two record-level rules: two banner lines, no joined summary, and no toast repeating them', async () => { const onSubmit = vi.fn().mockRejectedValue( ruleRefusal([{ field: '_record', message: AMOUNT }, { field: '_record', message: NAME }]), ); @@ -103,7 +96,8 @@ describe('form renderer — one entry per failed validation rule (#28053)', () = await waitFor(() => expect(container.querySelector('[role="alert"]')).toBeTruthy()); expect(bannerLines(container)).toEqual([AMOUNT, NAME]); - expect(toastLines()).toEqual([AMOUNT, NAME]); + // The banner is the refusal's one message (objectstack-ai/objectstack#27520). + expect(toastErrorSpy).not.toHaveBeenCalled(); expect(container.textContent).not.toContain('; '); }); @@ -119,7 +113,7 @@ describe('form renderer — one entry per failed validation rule (#28053)', () = await waitFor(() => expect(bestWrap.textContent).toContain(BEST)); expect(bestWrap.querySelector('[aria-invalid="true"]')).toBeTruthy(); expect(bannerLines(container)).toEqual([AMOUNT]); - expect(toastLines()).toEqual([AMOUNT]); + expect(toastErrorSpy).not.toHaveBeenCalled(); }); it('a single record-level rule is unchanged: its message, once', async () => { @@ -128,9 +122,9 @@ describe('form renderer — one entry per failed validation rule (#28053)', () = submit(); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled()); - expect(toastErrorSpy.mock.calls[0][0]).toBe(AMOUNT); + await waitFor(() => expect(container.querySelector('[role="alert"]')).toBeTruthy()); expect(bannerLines(container)).toEqual([AMOUNT]); + expect(toastErrorSpy).not.toHaveBeenCalled(); expect(container.querySelector('[aria-invalid="true"]')).toBeNull(); }); }); diff --git a/ui/packages/components/src/renderers/form/__tests__/form-server-field-errors.test.tsx b/ui/packages/components/src/renderers/form/__tests__/form-server-field-errors.test.tsx index 50d463af996..5777f24cc8d 100644 --- a/ui/packages/components/src/renderers/form/__tests__/form-server-field-errors.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/form-server-field-errors.test.tsx @@ -150,8 +150,10 @@ describe('form renderer — server-rejected fields', () => { submit(); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled()); - expect(toastErrorSpy.mock.calls[0][0]).toBe('Owner is required'); + // The banner is the refusal's one message (objectstack-ai/objectstack#27520). + const banner = await screen.findByRole('alert'); + expect(banner.textContent).toBe('Owner is required'); + expect(toastErrorSpy).not.toHaveBeenCalled(); }); it('a later client-side refusal retires the previous refusal`s banner and toast (objectstack#25874)', async () => { @@ -196,7 +198,7 @@ describe('form renderer — server-rejected fields', () => { submit(); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled()); + await screen.findByRole('alert'); // Nothing is attributable to a field, so no input is marked or scrolled to. expect(scrollSpy).not.toHaveBeenCalled(); }); @@ -242,8 +244,9 @@ describe('form renderer — unique-index refusal', () => { submit(); - await waitFor(() => expect(toastErrorSpy).toHaveBeenCalled()); - expect(String(toastErrorSpy.mock.calls[0][0])).toMatch(/A record with this value already exists/); + const banner = await screen.findByRole('alert'); + expect(banner.textContent).toMatch(/A record with this value already exists/); + expect(toastErrorSpy).not.toHaveBeenCalled(); expect(container.querySelector('[aria-invalid="true"]')).toBeNull(); }); }); diff --git a/ui/packages/components/src/renderers/form/__tests__/visibleWhen-fault-refuses-submit-8069.test.tsx b/ui/packages/components/src/renderers/form/__tests__/visibleWhen-fault-refuses-submit-8069.test.tsx index b7b28249dde..a48fce25303 100644 --- a/ui/packages/components/src/renderers/form/__tests__/visibleWhen-fault-refuses-submit-8069.test.tsx +++ b/ui/packages/components/src/renderers/form/__tests__/visibleWhen-fault-refuses-submit-8069.test.tsx @@ -98,7 +98,13 @@ function renderForm( } const submit = () => fireEvent.click(screen.getByRole('button', { name: /create/i })); -const lastToast = () => String(toastError.mock.calls.at(-1)?.[0] ?? ''); +/** + * The refusal as the form says it: its banner, the one message a refused + * submit raises; a toast repeating it is none of its business any more + * (objectstack-ai/objectstack#27520), so any toast call reads as a mismatch. + */ +const refusalText = () => + toastError.mock.calls.length > 0 ? '(toasted)' : (screen.queryByRole('alert')?.textContent ?? ''); /** A predicate the engine cannot parse: a fault whatever the record holds. */ const BROKEN = 'record.stage =='; @@ -116,7 +122,7 @@ describe('objectui#8069 — a faulted visibleWhen refuses the submit', () => { submit(); - await waitFor(() => expect(lastToast()).toMatch(/visibleWhen rule of Discount could not be evaluated/)); + await waitFor(() => expect(refusalText()).toMatch(/visibleWhen rule of Discount could not be evaluated/)); expect(screen.getByText(/visibleWhen rule of Discount could not be evaluated/)).toBeTruthy(); expect(onSubmit).not.toHaveBeenCalled(); }); @@ -133,7 +139,7 @@ describe('objectui#8069 — a faulted visibleWhen refuses the submit', () => { submit(); - await waitFor(() => expect(lastToast()).toMatch(/visibleWhen rule of Discount/)); + await waitFor(() => expect(refusalText()).toMatch(/visibleWhen rule of Discount/)); expect(toastError.mock.calls.some((c: unknown[]) => /check the highlighted fields/.test(String(c[0])))).toBe(false); expect(onSubmit).not.toHaveBeenCalled(); }); @@ -153,7 +159,7 @@ describe('objectui#8069 — a faulted visibleWhen refuses the submit', () => { renderForm(base({ visibleWhen: '' }), onSubmit); expect(screen.getByLabelText('Discount')).toBeTruthy(); submit(); - await waitFor(() => expect(lastToast()).toMatch(/visibleWhen rule of Discount could not be evaluated/)); + await waitFor(() => expect(refusalText()).toMatch(/visibleWhen rule of Discount could not be evaluated/)); expect(onSubmit).not.toHaveBeenCalled(); expect(warn.mock.calls.some((c: unknown[]) => String(c[0]).includes('[blank]'))).toBe(true); }); @@ -184,7 +190,7 @@ describe('objectui#8069 — a faulted visibleWhen refuses the submit', () => { const onSubmit = vi.fn().mockResolvedValue(undefined); renderForm(base({ visibleWhen: "previous.stage == 'won'" }), onSubmit); submit(); - await waitFor(() => expect(lastToast()).toMatch(/visibleWhen rule of Discount/)); + await waitFor(() => expect(refusalText()).toMatch(/visibleWhen rule of Discount/)); expect(onSubmit).not.toHaveBeenCalled(); }); diff --git a/ui/packages/components/src/renderers/form/form.tsx b/ui/packages/components/src/renderers/form/form.tsx index 12f6085aba1..32d30099346 100644 --- a/ui/packages/components/src/renderers/form/form.tsx +++ b/ui/packages/components/src/renderers/form/form.tsx @@ -1828,8 +1828,10 @@ ComponentRegistry.register('form', /** * The sonner id every OUTCOME toast of THIS form is published under. * - * The in-form banner (`submitError`) and its toast are raised together at - * each of the three points below, but only the banner was ever retired + * A refusal is no longer toasted at all (`raiseRefusal` below), but the id + * still retires whatever outcome toast an earlier attempt left. History: + * the in-form banner (`submitError`) and its toast were once raised together + * at each refusal point, but only the banner was ever retired * together: the toast went out under sonner's auto-generated id, so nothing * here held a handle on it and a refused submit's toast outlived the * attempt that raised it. A later SUCCESSFUL submit — whose success toast a @@ -1848,6 +1850,25 @@ ComponentRegistry.register('form', */ const formInstanceId = React.useId(); const outcomeToastId = `form-outcome:${formInstanceId}`; + /** + * A refused submit is said ONCE, by the banner above the fields + * (objectstack-ai/objectstack#27520). The toast that used to repeat it word + * for word was there for the banner a long modal or drawer form had + * scrolled out of view; instead the banner is brought into view each time + * it is raised, so no second copy of the refusal is needed. The stale + * toast of an earlier attempt is still dismissed under the outcome id. + */ + const submitBannerRef = React.useRef(null); + const [refusalRaised, setRefusalRaised] = React.useState(0); + const raiseRefusal = (message: string | string[]) => { + toast.dismiss(outcomeToastId); + setSubmitError(message); + setRefusalRaised((n) => n + 1); + }; + React.useEffect(() => { + if (refusalRaised === 0) return; + submitBannerRef.current?.scrollIntoView?.({ block: 'nearest' }); + }, [refusalRaised]); // Active `fieldTabs` panel. Undefined until something picks one — the // effective tab falls back to `defaultFieldTab`, then to the first tab. // Which tab of an in-progress form is showing is presentational and dies @@ -3051,10 +3072,7 @@ ComponentRegistry.register('form', // Check if submission returned an error if (result?.error) { - setSubmitError(result.error); - // Also surface as a toast so the message is visible even when the - // in-form banner has scrolled out of view (long forms in modals/drawers). - toast.error(result.error, { id: outcomeToastId }); + raiseRefusal(result.error); return; } } @@ -3140,10 +3158,7 @@ ComponentRegistry.register('form', if (isPermissionError(error) && typeof console !== 'undefined') { console.warn('[form] write denied:', error); } - setSubmitError(errorMessage); - // Also surface as a toast so the message is visible even when the - // in-form banner has scrolled out of view (long forms in modals/drawers). - toast.error(refusalContent(errorMessage), { id: outcomeToastId }); + raiseRefusal(errorMessage); // Log errors for debugging (dev environment only) // process may not be defined in all environments @@ -3209,14 +3224,12 @@ ComponentRegistry.register('form', const handleSubmit = (event?: React.BaseSyntheticEvent) => { if (faultedVisibleWhenFieldNames.length > 0) { event?.preventDefault?.(); - toast.dismiss(outcomeToastId); const message = t('form.visibleWhenFaulted', { fields: faultedVisibleWhenFieldNames .map((n) => fieldLabelByName[n] || n) .join(t('validation.formInvalidJoiner')), }); - setSubmitError(message); - toast.error(message, { id: outcomeToastId }); + raiseRefusal(message); // Marks the tabs that hold the fields, as a rejected submit does. setRejectedFieldNames(faultedVisibleWhenFieldNames); return Promise.resolve(); @@ -4076,7 +4089,7 @@ ComponentRegistry.register('form', > {/* Form Error Alert */} {submitError && ( - + {refusalContent(submitError)} diff --git a/ui/packages/plugin-form/src/MasterDetailForm.outcomeToastSupersede.test.tsx b/ui/packages/plugin-form/src/MasterDetailForm.outcomeToastSupersede.test.tsx index 27db82693b5..e072fabd9de 100644 --- a/ui/packages/plugin-form/src/MasterDetailForm.outcomeToastSupersede.test.tsx +++ b/ui/packages/plugin-form/src/MasterDetailForm.outcomeToastSupersede.test.tsx @@ -53,15 +53,11 @@ * decided, whether that retry succeeds or fails again. Driven, not assumed: * the second `it` below IS path 2, and it is green post-#7354. * - * The parent ``'s own renderer also toasts a rejected write, under - * its OWN `form-outcome:` id, and retires it at the top of the next submit - * (objectui#7252). Before objectui#7354 that toast was already superseded and - * was not what this file pinned; it was visible in the pre-fix registry below - * as the second error entry. Post-#7354 it is the ONLY error entry — the - * duplication objectui#7354 fixed is gone, so `distinctOnScreen` below now - * sees the same one-error reality `onScreen` does; see - * `MasterDetailForm.singleRefusalRaiser.test.tsx` for the pin that asserts - * that directly on the raw (undeduplicated) registry. + * The parent ``'s own renderer says a rejected write once, by + * its banner, and raises no error toast for it (objectstack-ai/objectstack#27520); + * it retires any outcome toast of its own at the top of the next submit + * (objectui#7252). `MasterDetailForm.singleRefusalRaiser.test.tsx` pins the + * one-message count directly. * * ## Why the pin models sonner rather than counting calls * @@ -189,27 +185,16 @@ const save = () => fireEvent.click(screen.getByRole('button', { name: /create/i const onScreen = (): ToastEntry[] => [...toastRegistry.values()]; /** - * What the screen SAYS, with each distinct statement counted once. - * - * Before objectui#7354: a refused master-detail save put the SAME refusal on - * screen twice — `handleError` here raised one, and the parent - * ``'s renderer raised its own for the write it re-threw. That - * duplication was a separate defect from the one THIS file pins, so the - * assertions below were written to deliberately not depend on HOW MANY - * raisers reported an outcome — only on WHICH outcomes remain readable. - * - * objectui#7354 fixed that duplication (removed `handleError`'s own toast), - * so today `distinctOnScreen()` and the raw `onScreen()` agree on a refused - * save — there is only the one raiser left. The dedup here is kept anyway: - * this file's OWN subject (a later outcome superseding an earlier one on THIS - * form) never depended on how many raisers were active, and - * `MasterDetailForm.singleRefusalRaiser.test.tsx` is where the raw, - * undeduplicated count is the point of the assertion. + * The refusal as the form says it: its banner, the one message a refused save + * raises (objectstack-ai/objectstack#27520) — no error toast repeats it. */ -const distinctOnScreen = (): ToastEntry[] => { - const seen = new Map(); - for (const t of onScreen()) seen.set(`${t.type}:${t.message}`, t); - return [...seen.values()]; +const refusalBanners = (): HTMLElement[] => + screen.queryAllByRole('alert').filter((el) => (el.textContent ?? '').includes(REFUSAL)); + +/** The refusal is said once, by its banner, and by no toast. */ +const saidOnceByBanner = () => { + expect(refusalBanners()).toHaveLength(1); + expect(onScreen().filter((t) => t.type === 'error')).toEqual([]); }; beforeEach(() => { @@ -228,7 +213,7 @@ describe('MasterDetailForm — a later save outcome supersedes this form’s ear fireEvent.change(await headerInput(container), { target: { value: 'PO-1' } }); save(); await waitFor(() => expect(ds.batchTransaction).toHaveBeenCalledTimes(1)); - await waitFor(() => expect(distinctOnScreen()).toEqual([{ type: 'error', message: REFUSAL }])); + await waitFor(saidOnceByBanner); // The user corrects the input and saves again inside the refusal's lifetime. fireEvent.change(await headerInput(container), { target: { value: 'PO-1b' } }); @@ -237,6 +222,7 @@ describe('MasterDetailForm — a later save outcome supersedes this form’s ear // The card's own sentence: one form, one outcome on screen. await waitFor(() => expect(onScreen()).toEqual([{ type: 'success', message: 'Created' }])); + expect(refusalBanners()).toEqual([]); }); it('retires the refusal even when the host owns confirmation (no built-in success toast)', async () => { @@ -251,7 +237,7 @@ describe('MasterDetailForm — a later save outcome supersedes this form’s ear fireEvent.change(await headerInput(container), { target: { value: 'PO-2' } }); save(); await waitFor(() => expect(ds.batchTransaction).toHaveBeenCalledTimes(1)); - await waitFor(() => expect(distinctOnScreen()).toEqual([{ type: 'error', message: REFUSAL }])); + await waitFor(saidOnceByBanner); fireEvent.change(await headerInput(container), { target: { value: 'PO-2b' } }); save(); @@ -260,6 +246,7 @@ describe('MasterDetailForm — a later save outcome supersedes this form’s ear // Nothing left asserting the save was refused. await waitFor(() => expect(onScreen()).toEqual([])); + expect(refusalBanners()).toEqual([]); }); it('does not stack a second refusal when the retry is refused too', async () => { @@ -273,15 +260,14 @@ describe('MasterDetailForm — a later save outcome supersedes this form’s ear fireEvent.change(await headerInput(container), { target: { value: 'PO-3' } }); save(); await waitFor(() => expect(ds.batchTransaction).toHaveBeenCalledTimes(1)); - await waitFor(() => expect(distinctOnScreen()).toEqual([{ type: 'error', message: REFUSAL }])); - // How many raisers reported this refusal is not this file's subject; that - // the SECOND attempt adds none of its own is. + await waitFor(saidOnceByBanner); + // That the SECOND attempt adds no message of its own is this file's subject. const afterFirstAttempt = onScreen().length; fireEvent.change(await headerInput(container), { target: { value: 'PO-3b' } }); save(); await waitFor(() => expect(ds.batchTransaction).toHaveBeenCalledTimes(2)); - await waitFor(() => expect(distinctOnScreen()).toEqual([{ type: 'error', message: REFUSAL }])); + await waitFor(saidOnceByBanner); expect(onScreen()).toHaveLength(afterFirstAttempt); }); }); diff --git a/ui/packages/plugin-form/src/MasterDetailForm.singleRefusalRaiser.test.tsx b/ui/packages/plugin-form/src/MasterDetailForm.singleRefusalRaiser.test.tsx index 49a07d0bbdd..585f1bfed9a 100644 --- a/ui/packages/plugin-form/src/MasterDetailForm.singleRefusalRaiser.test.tsx +++ b/ui/packages/plugin-form/src/MasterDetailForm.singleRefusalRaiser.test.tsx @@ -147,7 +147,7 @@ afterEach(() => { }); describe('MasterDetailForm — a refused save reports its refusal exactly once (objectui#7354)', () => { - it('leaves exactly one error toast on screen, not one per raiser', async () => { + it('says it once, by the form banner, with no error toast from any raiser', async () => { const ds = makeDataSource(); const { container } = masterDetail(ds); @@ -155,15 +155,16 @@ describe('MasterDetailForm — a refused save reports its refusal exactly once ( save(); await waitFor(() => expect(ds.batchTransaction).toHaveBeenCalledTimes(1)); - // Wait for the outcome to settle, then read the raw (undeduplicated) - // registry: this is the count that told the two-raiser defect apart from - // the fix. Give the throw its full trip through ``'s catch - // and back up into the form renderer's before asserting. - await waitFor(() => expect(onScreen().length).toBeGreaterThan(0)); - - const entries = onScreen(); - // The lit control: the surviving entry must still be the refusal, so this - // cannot pass by silencing every raiser instead of collapsing to one. - expect(entries).toEqual([{ type: 'error', message: REFUSAL }]); + // Wait for the outcome to settle: the throw's full trip through + // ``'s catch and back up into the form renderer's, which + // raises the banner (objectstack-ai/objectstack#27520). + await waitFor(() => + expect(screen.queryAllByRole('alert').filter((el) => (el.textContent ?? '').includes(REFUSAL))).toHaveLength(1), + ); + + // The raw (undeduplicated) registry: no raiser toasts the refusal as well. + // The lit control is the banner above, so this cannot pass by silencing + // every raiser. + expect(onScreen().filter((entry) => entry.type === 'error')).toEqual([]); }); }); diff --git a/ui/packages/plugin-form/src/MasterDetailForm.test.tsx b/ui/packages/plugin-form/src/MasterDetailForm.test.tsx index 453c5c434d0..2e58ea8d86b 100644 --- a/ui/packages/plugin-form/src/MasterDetailForm.test.tsx +++ b/ui/packages/plugin-form/src/MasterDetailForm.test.tsx @@ -128,7 +128,7 @@ describe('MasterDetailForm — submit feedback (never silent)', () => { expect(toastSuccess).not.toHaveBeenCalled(); }); - it('shows an error toast when the atomic write fails (rollback)', async () => { + it('says the failure when the atomic write fails (rollback)', async () => { const batchTransaction = vi.fn().mockRejectedValue(new Error('BATCH_ERROR: rolled back')); const ds = makeDataSource({ batchTransaction }); @@ -155,7 +155,8 @@ describe('MasterDetailForm — submit feedback (never silent)', () => { fireEvent.click(screen.getByRole('button', { name: /create/i })); await waitFor(() => expect(batchTransaction).toHaveBeenCalled()); - await waitFor(() => expect(toastError).toHaveBeenCalled()); // <- failure is surfaced + // The failure is surfaced, once, by the form's banner (objectstack-ai/objectstack#27520). + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()); expect(toastSuccess).not.toHaveBeenCalled(); }); }); @@ -211,7 +212,7 @@ describe('MasterDetailForm — non-atomic fallback (emulation via runBatchTransa expect(toastError).not.toHaveBeenCalled(); }); - it('create: a failing child create compensates by deleting the just-created parent + error toast', async () => { + it('create: a failing child create compensates by deleting the just-created parent + says the failure', async () => { const create = vi.fn(async (object: string, data: any) => { if (object === 'po_line') throw new Error('child create failed'); return { id: 'po1', ...data }; @@ -226,7 +227,7 @@ describe('MasterDetailForm — non-atomic fallback (emulation via runBatchTransa // Best-effort compensation removes the orphan parent that was created before // the child failed — the non-atomic path's stand-in for a rollback. await waitFor(() => expect(del).toHaveBeenCalledWith('po', 'po1')); - await waitFor(() => expect(toastError).toHaveBeenCalled()); + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()); expect(toastSuccess).not.toHaveBeenCalled(); }); }); diff --git a/ui/packages/plugin-form/src/WizardForm.outcomeToastSupersede.test.tsx b/ui/packages/plugin-form/src/WizardForm.outcomeToastSupersede.test.tsx index 3e6816345d6..352224f84d1 100644 --- a/ui/packages/plugin-form/src/WizardForm.outcomeToastSupersede.test.tsx +++ b/ui/packages/plugin-form/src/WizardForm.outcomeToastSupersede.test.tsx @@ -172,6 +172,17 @@ afterEach(() => { cleanup(); }); +/** + * The refusal as the form says it: its banner, the one message a refused + * submit raises (objectstack-ai/objectstack#27520) — no error toast repeats it. + */ +const refusalBanners = (): HTMLElement[] => + screen.queryAllByRole('alert').filter((el) => (el.textContent ?? '').includes(REFUSAL)); +const saidOnceByBanner = () => { + expect(refusalBanners()).toHaveLength(1); + expect(onScreen().filter((t) => t.type === 'error')).toEqual([]); +}; + describe('WizardForm — a later outcome supersedes this form’s earlier one', () => { it('leaves only the success toast after a refused submit is retried and accepted', async () => { const ds = makeDataSource(); @@ -185,7 +196,7 @@ describe('WizardForm — a later outcome supersedes this form’s earlier one', fireEvent.change(await inputNamed(container, 'status'), { target: { value: 'archived' } }); submitStep(container); await waitFor(() => expect(ds.create).toHaveBeenCalledTimes(1)); - await waitFor(() => expect(onScreen()).toEqual([{ type: 'error', message: REFUSAL }])); + await waitFor(saidOnceByBanner); // Back to step 2 of the reported flow, change the status, and Create again. // The navigation matters: it is what proves the id survives step changes — @@ -200,6 +211,7 @@ describe('WizardForm — a later outcome supersedes this form’s earlier one', // The card's own sentence: one form, one outcome on screen. await waitFor(() => expect(onScreen()).toEqual([{ type: 'success', message: SUCCESS }])); + expect(refusalBanners()).toEqual([]); }); it('does not stack a second refusal when the retry is refused too', async () => { @@ -216,10 +228,10 @@ describe('WizardForm — a later outcome supersedes this form’s earlier one', submitStep(container); await waitFor(() => expect(ds.create).toHaveBeenCalledTimes(1)); - await waitFor(() => expect(onScreen()).toHaveLength(1)); + await waitFor(saidOnceByBanner); submitStep(container); await waitFor(() => expect(ds.create).toHaveBeenCalledTimes(2)); - await waitFor(() => expect(onScreen()).toEqual([{ type: 'error', message: REFUSAL }])); + await waitFor(saidOnceByBanner); }); }); diff --git a/ui/packages/plugin-form/src/submitHandlerSeam.test.tsx b/ui/packages/plugin-form/src/submitHandlerSeam.test.tsx index c159aacad02..69e9f586c6b 100644 --- a/ui/packages/plugin-form/src/submitHandlerSeam.test.tsx +++ b/ui/packages/plugin-form/src/submitHandlerSeam.test.tsx @@ -181,9 +181,10 @@ describe('a failing child leg leaves NO committed parent, whichever variant rend // THE PIN — no committed parent survives the failed leg. await waitFor(() => expect(del).toHaveBeenCalledWith('po', 'po1')); - // …and the failure is surfaced. On the broken path the operator got a - // SUCCESS toast over a half-written document. - await waitFor(() => expect(toastError).toHaveBeenCalled()); + // …and the failure is surfaced, by the form's banner + // (objectstack-ai/objectstack#27520). On the broken path the operator got + // a SUCCESS toast over a half-written document. + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()); expect(toastSuccess).not.toHaveBeenCalled(); }, ); diff --git a/ui/packages/plugin-form/src/submitTargetRefusal.test.tsx b/ui/packages/plugin-form/src/submitTargetRefusal.test.tsx index fe6fcd63c13..b9f69104f01 100644 --- a/ui/packages/plugin-form/src/submitTargetRefusal.test.tsx +++ b/ui/packages/plugin-form/src/submitTargetRefusal.test.tsx @@ -535,8 +535,9 @@ describe('5. MasterDetailForm — the path that made this user-visible', () => { // so the seam is consulted first (block 1) and `submitViaBatch` gets to // state the precise reason. The renderer's generic refusal is what a form // with no seam at all falls through to (block 2). - await waitFor(() => expect(toastError).toHaveBeenCalledTimes(1)); - expect(String(toastError.mock.calls[0][0])).toBe('MasterDetailForm: dataSource is required'); + // Said once, by the form's banner (objectstack-ai/objectstack#27520). + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe('MasterDetailForm: dataSource is required')); + expect(toastError).not.toHaveBeenCalled(); expect(toastSuccess).not.toHaveBeenCalled(); // …and the submitter's input is still there. `handleSaved` bumps `formKey` diff --git a/ui/packages/plugin-grid/src/ObjectGrid.tsx b/ui/packages/plugin-grid/src/ObjectGrid.tsx index 8464ccafe5e..a41f5f5fc1d 100644 --- a/ui/packages/plugin-grid/src/ObjectGrid.tsx +++ b/ui/packages/plugin-grid/src/ObjectGrid.tsx @@ -84,6 +84,7 @@ import type { BulkActionDef } from '@object-ui/types'; import { type ObjectGridColumnDraft } from './objectGridColumns'; import { rowRecordId } from './objectGridLinkCell'; import { useGridTranslation } from './objectGridTranslations'; +import { inlineEditRuleRefusals, inlineRuleRefusalError } from './objectGridInlineRules'; import { DEFAULT_DISPLAY_PAGE_SIZE, DEFAULT_SELECTION_TYPE, type RowHeightMode, resolvePageSize, resolveRowHeightMode } from './objectGridDefaults'; import { type BatchSaveOutcome, type RowConflictChoice, type WrittenRowVersion, rowVersionOf, rowVersionToSend } from './objectGridRowVersion'; import { type ObjectGridExternalPaginationProps, type ObjectGridFooterSummary } from './objectGridProps'; @@ -2299,6 +2300,22 @@ export const ObjectGrid: React.FC = ({ return 'saved'; }; + /** + * The field rules a row's edits break, refused before anything is sent + * (objectstack-ai/objectstack#27520): the record form's own rules, judged + * here so a value the form would refuse never reaches the server. `null` + * when the edits keep every rule. + */ + const inlineRuleRefusal = (changes: Record): Error | null => { + const refusals = inlineEditRuleRefusals( + changes, + objectSchema?.fields, + (field) => resolveFieldLabel(objectName ?? '', field, objectSchema?.fields?.[field]?.label || field), + t, + ); + return refusals.length > 0 ? inlineRuleRefusalError(refusals) : null; + }; + const defaultRowSave = async ( _rowIndex: number, changes: Record, @@ -2311,6 +2328,8 @@ export const ObjectGrid: React.FC = ({ if (id === undefined || id === null) { throw new Error('Cannot persist inline edit: row has no id/_id.'); } + const refused = inlineRuleRefusal(changes); + if (refused) throw refused; // objectstack-ai/objectstack#28252 — the row's write answer feedback (its // warnings and its hook's notice) rides the save toast below, so the // write is one toast. @@ -2377,6 +2396,10 @@ export const ObjectGrid: React.FC = ({ if (id === undefined || id === null) { return Promise.reject(new Error('Cannot persist inline edit: row has no id/_id.')); } + // A row whose edits break a field rule is refused here, unsent; the + // other rows still go (each row is its own outcome). + const refused = inlineRuleRefusal(rowChanges); + if (refused) return Promise.reject(refused); return writeRowGuarded(dataSource, objectName, id, rowChanges, row); }), ); @@ -2720,7 +2743,9 @@ export const ObjectGrid: React.FC = ({ // author writes `true`, and an explicit `false` turns it off on an editable // grid. Not an alias fallback: the right side is the key's own default. keyboardNavigation: schema.keyboardNavigation ?? inlineEditable, - className: schema.className, + // The save bar sits on the grid's own inset, as its cells do (`px-3`), + // never flush against the view's edge (objectstack-ai/objectstack#27520). + className: ['[&>[data-slot=table-toolbar]]:px-3 [&>[data-slot=table-toolbar]]:pt-2', schema.className].filter(Boolean).join(' '), cellClassName: rowHeightMode === 'compact' ? 'px-3 py-1 text-[13px] leading-tight' : rowHeightMode === 'short' diff --git a/ui/packages/plugin-grid/src/__tests__/inlineEditRuleCheck-27520.test.tsx b/ui/packages/plugin-grid/src/__tests__/inlineEditRuleCheck-27520.test.tsx new file mode 100644 index 00000000000..4425e30ca20 --- /dev/null +++ b/ui/packages/plugin-grid/src/__tests__/inlineEditRuleCheck-27520.test.tsx @@ -0,0 +1,203 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectstack-ai/objectstack#27520 (row 6) — the grid's default inline saves + * check the record form's field rules before sending. + * + * Measured on the card: Tasks › Grid, Title cleared and Estimate set to -5 + * (`min: 0`), "Save All". The `PATCH` went out with -5 and came back refused. + * Now a row whose edits break a declared rule is refused here, unsent, in the + * shape the server refuses it in, so the table marks its cells and names its + * reasons; the other rows still save. + * + * The real `ObjectGrid` and `data-table` over an in-memory data source whose + * `update` persists. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, fireEvent, cleanup, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +const toasts = vi.hoisted(() => ({ success: vi.fn(), error: vi.fn(), info: vi.fn(), warning: vi.fn() })); +vi.mock('@object-ui/components', async (importOriginal) => { + const actual = await importOriginal>(); + return { ...actual, toast: Object.assign(vi.fn(), toasts) }; +}); + +import { registerAllFields } from '@object-ui/fields'; +import { ActionProvider } from '@object-ui/react'; +import { ObjectGrid } from '../ObjectGrid'; +import { __clearRecordCrudVerdictCache } from '../hooks/useRecordCrudVerdicts'; +import { installExplainDouble } from './explainDouble'; +import { inlineEditRuleRefusals } from '../objectGridInlineRules'; + +registerAllFields(); + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as unknown as Element['scrollIntoView']; + } +}); +beforeEach(() => { + __clearRecordCrudVerdictCache(); + installExplainDouble(); + for (const fn of Object.values(toasts)) fn.mockClear(); +}); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +const OBJECT = 'task_27520'; + +const FIELDS = { + id: { type: 'text' }, + title: { type: 'text', label: 'Title', required: true, maxLength: 20 }, + estimate_hours: { type: 'number', label: 'Estimate (h)', min: 0, max: 1000 }, +}; + +function makeDataSource() { + const store: Record> = { + t1: { id: 't1', title: 'Ingest pipeline', estimate_hours: 60 }, + t2: { id: 't2', title: 'Warehouse schema', estimate_hours: 33 }, + }; + return { + store, + find: vi.fn(async () => { + const data = Object.values(store).map((r) => ({ ...r })); + return { data, total: data.length, hasMore: false, pageSize: 50 }; + }), + update: vi.fn(async (_object: string, id: string, changes: Record) => { + store[id] = { ...store[id], ...changes }; + return { ...store[id] }; + }), + getObjectSchema: vi.fn(async (name: string) => ({ name, fields: FIELDS })), + }; +} + +type GridProps = React.ComponentProps; + +async function loaded(dataSource: ReturnType) { + const schema = { + type: 'object-grid', + objectName: OBJECT, + editable: true, + singleClickEdit: true, + columns: [ + { field: 'title', label: 'Title', editable: false }, + { field: 'estimate_hours', label: 'Estimate (h)', type: 'number' }, + ], + pagination: { pageSize: 50 }, + } as GridProps['schema']; + render( + + {}} + onDelete={() => {}} + /> + , + ); + await waitFor(() => expect(screen.getByText('Ingest pipeline')).toBeInTheDocument()); +} + +const rowOf = (title: string) => screen.getByText(title).closest('tr') as HTMLElement; + +async function editEstimate(title: string, from: string, to: string) { + const td = await waitFor(() => { + const cell = Array.from(rowOf(title).querySelectorAll('td')).find((c) => c.textContent?.trim() === from); + if (!cell) throw new Error(`no ${from} cell on ${title}`); + return cell as HTMLElement; + }); + fireEvent.click(td); + const input = await waitFor(() => { + const el = td.querySelector('input') as HTMLInputElement | null; + if (!el) throw new Error('editor not open'); + return el; + }); + fireEvent.change(input, { target: { value: to } }); + fireEvent.keyDown(input, { key: 'Enter' }); + await waitFor(() => expect(within(rowOf(title)).getByText(to)).toBeInTheDocument()); + return td; +} + +describe('ObjectGrid default saves — a field rule is checked before sending (#27520)', () => { + it('"Save All": the row below `min` is refused unsent, its cell marked; the other row saves', async () => { + const ds = makeDataSource(); + await loaded(ds); + const refusedCell = await editEstimate('Ingest pipeline', '60', '-5'); + await editEstimate('Warehouse schema', '33', '31'); + fireEvent.click(screen.getByRole('button', { name: /Save All/ })); + + await waitFor(() => + expect(screen.getByTestId('table-save-refusal')).toHaveTextContent('Estimate (h) must be at least 0'), + ); + // Only the row that keeps every rule was sent. + expect(ds.update).toHaveBeenCalledTimes(1); + expect(ds.update).toHaveBeenCalledWith(OBJECT, 't2', { estimate_hours: 31 }, undefined); + expect(ds.store.t1.estimate_hours).toBe(60); + expect(refusedCell).toHaveAttribute('aria-invalid', 'true'); + expect(refusedCell).toHaveAttribute('data-cell-error', 'Estimate (h) must be at least 0'); + expect(toasts.success).toHaveBeenCalledWith('1 row saved'); + }); + + it('the per-row save refuses the same edit unsent', async () => { + const ds = makeDataSource(); + await loaded(ds); + await editEstimate('Ingest pipeline', '60', '-7'); + fireEvent.click(within(rowOf('Ingest pipeline')).getByTitle('Save row')); + + await waitFor(() => + expect(screen.getByTestId('table-save-refusal')).toHaveTextContent('Estimate (h) must be at least 0'), + ); + expect(ds.update).not.toHaveBeenCalled(); + expect(toasts.success).not.toHaveBeenCalled(); + }); + + it('CONTROL — an edit inside the rules is sent as before', async () => { + const ds = makeDataSource(); + await loaded(ds); + await editEstimate('Ingest pipeline', '60', '0'); + fireEvent.click(screen.getByRole('button', { name: /Save All/ })); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(screen.queryByTestId('table-save-refusal')).toBeNull(); + }); +}); + +describe('inlineEditRuleRefusals — the record form’s rules, one entry per broken rule', () => { + const t = (key: string, o?: Record) => `${key}|${JSON.stringify(o ?? {})}`; + const label = (f: string) => (FIELDS as Record)[f]?.label ?? f; + + it('names each broken rule with its field and the localized message', () => { + expect(inlineEditRuleRefusals({ title: '', estimate_hours: -5 }, FIELDS, label, t)).toEqual([ + { field: 'title', code: 'required', message: 'validation.required|{"field":"Title"}' }, + { field: 'estimate_hours', code: 'min_value', message: 'validation.min|{"field":"Estimate (h)","min":0}' }, + ]); + }); + + it('reads a staged number string the way the server will read it, and judges length on text', () => { + expect(inlineEditRuleRefusals({ estimate_hours: '-1' }, FIELDS, label, t).map((r) => r.code)).toEqual(['min_value']); + expect(inlineEditRuleRefusals({ title: 'x'.repeat(21) }, FIELDS, label, t).map((r) => r.code)).toEqual(['max_length']); + }); + + it('judges nothing it has no declaration for, and passes values inside the rules', () => { + expect(inlineEditRuleRefusals({ estimate_hours: 0, title: 'ok' }, FIELDS, label, t)).toEqual([]); + expect(inlineEditRuleRefusals({ unknown: -5 }, FIELDS, label, t)).toEqual([]); + expect(inlineEditRuleRefusals({ estimate_hours: -5 }, undefined, label, t)).toEqual([]); + }); + + it('an authored rule message wins over the localized default', () => { + const fields = { qty: { type: 'number', label: 'Qty', min: 1, min_message: 'Order at least one.' } }; + expect(inlineEditRuleRefusals({ qty: 0 }, fields, label, t)).toEqual([ + { field: 'qty', code: 'min_value', message: 'Order at least one.' }, + ]); + }); +}); diff --git a/ui/packages/plugin-grid/src/objectGridInlineRules.ts b/ui/packages/plugin-grid/src/objectGridInlineRules.ts new file mode 100644 index 00000000000..8b71131ea3d --- /dev/null +++ b/ui/packages/plugin-grid/src/objectGridInlineRules.ts @@ -0,0 +1,130 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The field rules an inline grid edit is checked against before it is sent + * (objectstack-ai/objectstack#27520): the ones the record form checks, read + * through the same `buildValidationRules` — `required`, `min` / `max`, + * `minLength` / `maxLength` and the email / URL pattern — so a value the form + * would refuse is refused here too, by the same rule, without a round trip. + * + * A row that breaks one is refused in the shape the server refuses it in, a + * `VALIDATION_FAILED` error carrying one `fields[]` entry per broken rule, so + * the table marks its cells and says its reasons exactly as it does for a + * server refusal. The server stays the judge: everything it checks that is not + * listed above (validation rules, uniqueness, state machines) still reaches it. + */ +import { buildValidationRules } from '@object-ui/fields'; + +/** One rule an edited value breaks: the server's `fields[]` entry shape. */ +export interface InlineRuleRefusal { + field: string; + code: string; + message: string; +} + +type Translate = (key: string, options?: Record) => string; + +/** A rule as `buildValidationRules` emits it: an authored message, or a key the caller localizes. */ +interface EmittedRule { + value?: unknown; + message?: string; + messageKey?: string; +} + +const isBlank = (value: unknown): boolean => + value === undefined || value === null || (typeof value === 'string' && value.trim() === '') || + (Array.isArray(value) && value.length === 0); + +/** The number an inline number editor staged, or `undefined` when it is not one. */ +function numericValue(value: unknown): number | undefined { + if (typeof value === 'number') return Number.isFinite(value) ? value : undefined; + if (typeof value === 'string' && value.trim() !== '') { + const n = Number(value); + return Number.isFinite(n) ? n : undefined; + } + return undefined; +} + +function ruleMessage( + rule: EmittedRule, + t: Translate, + label: string, + interp: Record = {}, +): string { + if (typeof rule.message === 'string' && rule.message) return rule.message; + return t(rule.messageKey ?? 'validation.pattern', { field: label, ...interp }); +} + +/** + * The rules `changes` breaks, one entry per broken rule, judged against each + * changed field's declaration in `fields` (a field with no declaration is not + * judged). `labelOf` names a field the way the grid's header does. + */ +export function inlineEditRuleRefusals( + changes: Record, + fields: Record | null | undefined, + labelOf: (field: string) => string, + t: Translate, +): InlineRuleRefusal[] { + const refusals: InlineRuleRefusal[] = []; + if (!fields) return refusals; + for (const [field, value] of Object.entries(changes)) { + const def = fields[field]; + if (!def || typeof def !== 'object') continue; + // A rule set that cannot be read judges nothing: the server stays the judge. + const rules = (buildValidationRules(def) ?? {}) as Record; + const label = labelOf(field); + if (isBlank(value)) { + if (rules.required) { + refusals.push({ + field, + code: 'required', + message: typeof rules.required === 'string' ? rules.required : t('validation.required', { field: label }), + }); + } + continue; + } + const min = rules.min as EmittedRule | undefined; + const max = rules.max as EmittedRule | undefined; + const n = numericValue(value); + if (n !== undefined && min && typeof min.value === 'number' && n < min.value) { + refusals.push({ field, code: 'min_value', message: ruleMessage(min, t, label, { min: min.value }) }); + } + if (n !== undefined && max && typeof max.value === 'number' && n > max.value) { + refusals.push({ field, code: 'max_value', message: ruleMessage(max, t, label, { max: max.value }) }); + } + if (typeof value === 'string') { + const minLength = rules.minLength as EmittedRule | undefined; + const maxLength = rules.maxLength as EmittedRule | undefined; + const pattern = rules.pattern as (EmittedRule & { value?: RegExp }) | undefined; + if (minLength && typeof minLength.value === 'number' && value.length < minLength.value) { + refusals.push({ field, code: 'min_length', message: ruleMessage(minLength, t, label, { min: minLength.value }) }); + } + if (maxLength && typeof maxLength.value === 'number' && value.length > maxLength.value) { + refusals.push({ field, code: 'max_length', message: ruleMessage(maxLength, t, label, { max: maxLength.value }) }); + } + if (pattern && pattern.value instanceof RegExp && !pattern.value.test(value)) { + refusals.push({ field, code: 'pattern', message: ruleMessage(pattern, t, label) }); + } + } + } + return refusals; +} + +/** + * A row refused before it was sent, in the shape the server's refusal arrives + * in: `code: 'VALIDATION_FAILED'`, the reasons `; `-joined on `message`, and + * the entries on `fields`. + */ +export function inlineRuleRefusalError(refusals: InlineRuleRefusal[]): Error { + return Object.assign(new Error(refusals.map((r) => r.message).join('; ')), { + code: 'VALIDATION_FAILED', + fields: refusals, + }); +} diff --git a/ui/packages/plugin-grid/src/objectGridTranslations.ts b/ui/packages/plugin-grid/src/objectGridTranslations.ts index c2b2b0fb625..7c4011ae8e2 100644 --- a/ui/packages/plugin-grid/src/objectGridTranslations.ts +++ b/ui/packages/plugin-grid/src/objectGridTranslations.ts @@ -52,6 +52,18 @@ const GRID_DEFAULT_TRANSLATIONS: Record = { 'table.saved': '{{count}} row saved', 'table.saved_one': '{{count}} row saved', 'table.saved_other': '{{count}} rows saved', + // The field rules an inline edit is checked against before it is sent + // (objectstack-ai/objectstack#27520), worded as the record form words them. + // Byte-identical to the `en` pack, which `defaults-maps-mirror-en-pack` + // enforces. + 'validation.required': '{{field}} is required', + 'validation.minLength': '{{field}} must be at least {{min}} characters', + 'validation.maxLength': '{{field}} must be at most {{max}} characters', + 'validation.min': '{{field}} must be at least {{min}}', + 'validation.max': '{{field}} must be at most {{max}}', + 'validation.email': 'Please enter a valid email address', + 'validation.url': 'Please enter a valid URL', + 'validation.pattern': '{{field}} format is invalid', // The heading of an authored `emptyState` that declares no `title` // (objectui#11068): the words the table's own empty row shows, so leaving the // member out changes nothing a user reads.