From 3475ac3cc93ce91d2727964c9759dc8a47719449 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 23:52:16 +0000 Subject: [PATCH 1/6] fix(plugin-view): make the grid's row and bulk Delete delete on the registered object-view path On the registered `object-view` renderer (no host list view), the grid's row Delete and bulk Delete only bumped the refresh counter: `ObjectGrid` hands the row to the consumer's `onDelete` / `onBulkDelete` and performs no delete itself, so a Delete offered by default called `dataSource.delete` zero times and the row came back. The two handlers now do what the console's own list does with the same callbacks: confirm (`objectActions.deleteConfirm` for a row, `console.objectView.bulkDeleteConfirm` once for a selection, in the `actionConfirm.*` dialog chrome), then `dataSource.delete` per record, refresh, and the console's success / failure toasts. Whether the affordance is offered stays `ObjectGrid`'s verdict, as on the console list. Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- packages/plugin-view/src/ObjectView.tsx | 139 +++++++- .../ObjectView.gridDelete-10383.test.tsx | 335 ++++++++++++++++++ 2 files changed, 468 insertions(+), 6 deletions(-) create mode 100644 packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index 0e3324f917..f011d9f832 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -55,6 +55,15 @@ import { TabsList, TabsTrigger, useIsMobile, + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, + toast, } from '@object-ui/components'; import { Plus } from 'lucide-react'; import { useObjectTranslation, createSafeTranslation } from '@object-ui/i18n'; @@ -288,6 +297,21 @@ const VIEW_DEFAULT_TRANSLATIONS: Record = { 'form.createTitle': 'Create {{object}}', 'form.editTitle': 'Edit {{object}}', 'form.viewTitle': 'View {{object}}', + // objectui#10383 — the grid's row / bulk Delete. Not new keys: these are the + // ones the console's own list delete resolves (`useObjectActions` for the + // confirm text and the toasts, the console `ObjectView` for the bulk confirm, + // `ActionConfirmDialog` for the dialog chrome), all present in the ten packs. + // Reusing them is what keeps this path's dialog and toasts word-for-word the + // console list's, in every language. + 'actionConfirm.title': 'Confirm Action', + 'actionConfirm.confirm': 'Continue', + 'actionConfirm.cancel': 'Cancel', + 'objectActions.deleteConfirm': 'Are you sure you want to delete this record?', + 'console.objectView.bulkDeleteConfirm': 'Delete {{count}} selected records? This cannot be undone.', + 'objectActions.deleteSuccess': '{{label}} deleted successfully', + 'objectActions.deleteFailed': 'Failed to delete {{label}}', + 'objectActions.bulkDeleteSuccess': 'Deleted {{count}} {{label}} records', + 'objectActions.bulkDeletePartial': '{{succeeded}} deleted, {{failed}} failed', }; const useObjectViewTranslation = createSafeTranslation( @@ -1333,16 +1357,78 @@ export const ObjectView: React.FC = ({ } }, [onRowClick, navigationConfig, operations.read, handleView, openRecordInNewTab, schema]); - // Handle delete action - const handleDelete = useCallback((_record: Record) => { - setRefreshKey(prev => prev + 1); + // Handle delete / bulk delete — objectui#10383. + // + // `ObjectGrid` hands the row (or the selection) straight to these two and + // performs no delete of its own: its contract is that the CONSUMER's delete + // flow owns the confirmation, the delete, the toast and the refresh. Both + // handlers used to ignore their argument and only bump `refreshKey`, so on + // this path — the registered `object-view` renderer, no host list view — a + // Delete offered by default deleted nothing and the row came back. + // + // They now do what the console's own list does with the same two callbacks + // (`app-shell` `ObjectView` → `useObjectActions`), step for step: + // - a row asks `objectActions.deleteConfirm`; a selection asks + // `console.objectView.bulkDeleteConfirm` once for the whole batch; + // - a row without an `id` is skipped, as the console skips it; + // - one record: `dataSource.delete`, then refresh + `deleteSuccess`, or the + // `deleteFailed` toast with the error's message and NO refresh; + // - several: `dataSource.delete` per record, settled together (the console + // does not use the optional `bulkDelete`), then refresh, then + // `bulkDeleteSuccess` or `bulkDeletePartial`. + // The permission half needs nothing here: whether the Delete affordance is + // offered at all is `ObjectGrid`'s verdict on both paths (`operations`, the + // principal's `can(object, 'delete')`, the object's bucket / `userActions` / + // API operations and the per-record explain verdict), exactly as it is for + // the console list, which does not gate its handlers either. + const [deleteRequest, setDeleteRequest] = useState<{ + open: boolean; + records: Record[]; + bulk: boolean; + } | null>(null); + + const handleDelete = useCallback((record: Record) => { + if (record?.id == null) return; + setDeleteRequest({ open: true, records: [record], bulk: false }); }, []); - // Handle bulk delete action - const handleBulkDelete = useCallback((_records: Record[]) => { - setRefreshKey(prev => prev + 1); + const handleBulkDelete = useCallback((records: Record[]) => { + const valid = records.filter((r) => r?.id != null); + if (valid.length === 0) return; + setDeleteRequest({ open: true, records: valid, bulk: true }); }, []); + const performDelete = async (records: Record[]) => { + const objectName = schema.objectName; + const label = (objectSchema?.label as string) || objectName; + if (records.length > 1) { + // `async` callback on purpose: a throw inside it becomes a rejected + // settlement and is counted as a failure, instead of escaping `map` + // before `allSettled` ever sees the batch. + const results = await Promise.allSettled( + records.map(async (r) => dataSource.delete(objectName, String(r.id))), + ); + const failed = results.filter((r) => r.status === 'rejected').length; + const succeeded = results.length - failed; + setRefreshKey(prev => prev + 1); + if (failed === 0) { + toast.success(tView('objectActions.bulkDeleteSuccess', { count: succeeded, label })); + } else { + toast.error(tView('objectActions.bulkDeletePartial', { succeeded, failed })); + } + return; + } + try { + await dataSource.delete(objectName, String(records[0].id)); + setRefreshKey(prev => prev + 1); + toast.success(tView('objectActions.deleteSuccess', { label })); + } catch (err) { + toast.error(tView('objectActions.deleteFailed', { label }), { + description: (err as Error | undefined)?.message, + }); + } + }; + // Handle form submission const handleFormSuccess = useCallback(() => { setIsFormOpen(false); @@ -2509,6 +2595,45 @@ export const ObjectView: React.FC = ({ // empty `mb-4` spacer div above the content. const toolbar = renderToolbar(); + // The delete confirmation (objectui#10383) — the console's + // `ActionConfirmDialog` shape and keys: `actionConfirm.*` chrome, the + // question as the description. Close flips `open` and KEEPS the request, so + // the description does not blank during the exit animation (the objectui#6034 + // lesson); the `open` guard on Continue is what stops a click during that + // animation from deleting twice. + const deleteConfirmDialog = ( + { + if (!open) setDeleteRequest((prev) => (prev ? { ...prev, open: false } : prev)); + }} + > + + + {tView('actionConfirm.title')} + + {deleteRequest?.bulk + ? tView('console.objectView.bulkDeleteConfirm', { count: deleteRequest.records.length }) + : tView('objectActions.deleteConfirm')} + + + + {tView('actionConfirm.cancel')} + { + if (!deleteRequest?.open) return; + const { records } = deleteRequest; + setDeleteRequest({ ...deleteRequest, open: false }); + void performDelete(records); + }} + > + {tView('actionConfirm.confirm')} + + + + + ); + // For split mode, wrap content inside NavigationOverlay with mainContent if (formLayout === 'split') { const objectLabel = (objectSchema?.label as string) || schema.objectName; @@ -2549,6 +2674,7 @@ export const ObjectView: React.FC = ({ renderContent() )} + {deleteConfirmDialog} ); } @@ -2593,6 +2719,7 @@ export const ObjectView: React.FC = ({ {renderOverlayDetail} )} + {deleteConfirmDialog} ); }; diff --git a/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx new file mode 100644 index 0000000000..cc9bad3713 --- /dev/null +++ b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx @@ -0,0 +1,335 @@ +/** + * 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. + */ + +/** + * objectui#10383 — on the registered `object-view` path (no host list view), + * the grid's row Delete and bulk Delete really delete. + * + * ## The defect this pins + * + * `ObjectGrid` hands the row (or the selection) to the consumer's `onDelete` / + * `onBulkDelete` and performs no delete itself — the consumer owns confirm, + * delete, toast and refresh. `ObjectView`'s two handlers ignored their argument + * and only bumped the refresh counter, so a Delete the grid offers by default + * called `dataSource.delete` zero times and the row came back after the + * refresh. Measured against the pre-fix source with the REAL `ObjectGrid`: + * row Delete and bulk Delete, `dataSource.delete` = 0 calls each. + * + * ## What "fixed" means here — the console list's behaviour + * + * The console's own list (`app-shell` `ObjectView` → `useObjectActions`) is + * the behaviour matched, and every case below reads one of its steps: + * - a row asks "Are you sure you want to delete this record?"; a selection + * asks "Delete N selected records? This cannot be undone." once; + * - Continue deletes through `dataSource.delete`, one call per record, and the + * row is gone after the refresh; Cancel deletes nothing; + * - success toasts `{{label}} deleted successfully` / + * `Deleted N {{label}} records`; a failure toasts `Failed to delete + * {{label}}` (with the error message) or `N deleted, M failed`; + * - whether the affordance is offered at all is `ObjectGrid`'s own verdict, + * the same one the console list rides: a principal without the delete grant + * sees neither the row Delete nor the bulk Delete on this path. + * + * Every case drives the real `ObjectGrid` through `ObjectView`, the way the + * registered renderer composes them, against an in-memory data source whose + * `delete` really removes the row — so "the row is gone" is read off the + * refetched grid, not inferred from a spy. + */ + +import React from 'react'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, fireEvent, cleanup, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import '@testing-library/jest-dom'; + +// Stable stub identity (ObjectGrid carries `perms` in memo dependency arrays). +// `noProvider: true` runs the REAL provider-less hook — the fail-open path a +// standalone `object-view` runs on — and is every case's default; only the +// permission cases swap in the stub. +const { permsStub, permState } = vi.hoisted(() => { + const permState = { permDelete: true, noProvider: true }; + return { + permState, + permsStub: { + isLoaded: false, + checkField: () => true, + getObjectApiOperations: () => undefined, + can: (_obj: string, action: string) => + action === 'delete' ? permState.permDelete : true, + }, + }; +}); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + usePermissions: () => { + const real = actual.usePermissions(); + return permState.noProvider ? real : permsStub; + }, + }; +}); + +import { toast } from '@object-ui/components'; +import { ActionProvider, SchemaRendererProvider } from '@object-ui/react'; +import type { DataSource } from '@object-ui/types'; +import { ObjectView } from '../ObjectView'; +import { installExplainDouble } from './explainDouble'; + +const OBJECT = 'test_object'; + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +let successSpy: ReturnType; +let errorSpy: ReturnType; + +beforeEach(() => { + permState.permDelete = true; + permState.noProvider = true; + installExplainDouble(); + successSpy = vi.spyOn(toast, 'success').mockImplementation(() => 'toast-id' as never); + errorSpy = vi.spyOn(toast, 'error').mockImplementation(() => 'toast-id' as never); +}); + +afterEach(() => { + // Unmount before restoring the real `fetch` (objectui#7439). + cleanup(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +/** + * An in-memory data source: `find` answers the CURRENT store, `delete` removes + * the row — unless its id is listed in `failIds`, which rejects instead. + */ +function makeDataSource(failIds: string[] = []) { + let store = [ + { id: 'r1', name: 'Alice' }, + { id: 'r2', name: 'Bob' }, + { id: 'r3', name: 'Carol' }, + ]; + const ds = { + find: vi.fn(async () => ({ data: store.map((r) => ({ ...r })), total: store.length })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(async (_object: string, id: string) => { + if (failIds.includes(id)) throw new Error(`refused ${id}`); + store = store.filter((r) => r.id !== id); + return true; + }), + // Declared by the contract as optional; the console does not use it, so + // neither does this path. Present here so a call would be observable. + bulkDelete: vi.fn(async () => 0), + getObjectSchema: vi.fn(async () => ({ + name: OBJECT, + label: 'Test object', + fields: { id: { type: 'text' }, name: { type: 'text', label: 'Name' } }, + })), + }; + return ds; +} + +function renderView(ds: ReturnType, extra: Record = {}) { + return render( + + + + + , + ); +} + +/** Wait for the rows, and for the object-schema-derived affordances to settle. */ +async function settled() { + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + await waitFor(() => + expect(document.querySelectorAll('[role="checkbox"]').length).toBeGreaterThan(3), + ); +} + +/** Open the row kebab of the row showing `label`; returns its Delete entry, or null. */ +async function openRowMenu(label: string): Promise { + const row = screen.getByText(label).closest('tr'); + const trigger = row?.querySelector('[data-testid="row-action-trigger"]'); + if (!trigger) return null; + await userEvent.click(trigger); + return screen.queryByTestId('row-action-builtin-delete'); +} + +/** + * What the row kebab of `label` offers. Read by ROW, not by index, and only + * after the menu opened — so "no Delete" is a reading of an open menu that + * still offers Edit, never of a menu that did not open. + */ +async function rowMenuEntries(label: string): Promise<{ edit: boolean; delete: boolean }> { + const row = screen.getByText(label).closest('tr'); + const trigger = row?.querySelector('[data-testid="row-action-trigger"]'); + if (!trigger) return { edit: false, delete: false }; + await userEvent.click(trigger); + await screen.findByTestId('row-action-builtin-edit'); + const answer = { + edit: screen.queryAllByTestId('row-action-builtin-edit').length > 0, + delete: screen.queryAllByTestId('row-action-builtin-delete').length > 0, + }; + await userEvent.keyboard('{Escape}'); + return answer; +} + +/** Tick the rows showing `labels` (checkbox 0 is the header's select-all). */ +function selectRows(labels: string[]) { + for (const label of labels) { + const row = screen.getByText(label).closest('tr') as HTMLElement; + fireEvent.click(within(row).getByRole('checkbox')); + } +} + +async function confirmDialog(): Promise { + return screen.findByRole('alertdialog'); +} + +describe('objectui#10383 — row Delete on the registered object-view path', () => { + it('confirms, calls dataSource.delete for the row, and the row is gone after the refresh', async () => { + const ds = makeDataSource(); + renderView(ds); + await settled(); + + const deleteItem = await openRowMenu('Alice'); + expect(deleteItem, 'the row kebab offers no Delete — the harness is not reaching the subject').not.toBeNull(); + await userEvent.click(deleteItem!); + + const dialog = await confirmDialog(); + expect(within(dialog).getByText('Are you sure you want to delete this record?')).toBeInTheDocument(); + // Nothing is deleted before the user answers. + expect(ds.delete).not.toHaveBeenCalled(); + + await userEvent.click(within(dialog).getByRole('button', { name: 'Continue' })); + + await waitFor(() => expect(ds.delete).toHaveBeenCalledTimes(1)); + expect(ds.delete).toHaveBeenCalledWith(OBJECT, 'r1'); + await waitFor(() => expect(screen.queryByText('Alice')).not.toBeInTheDocument()); + expect(screen.getByText('Bob')).toBeInTheDocument(); + expect(successSpy).toHaveBeenCalledWith('Test object deleted successfully'); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('Cancel deletes nothing and the row stays', async () => { + const ds = makeDataSource(); + renderView(ds); + await settled(); + + await userEvent.click((await openRowMenu('Alice'))!); + const dialog = await confirmDialog(); + await userEvent.click(within(dialog).getByRole('button', { name: 'Cancel' })); + + await waitFor(() => expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument()); + expect(ds.delete).not.toHaveBeenCalled(); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect(successSpy).not.toHaveBeenCalled(); + }); + + it('a refused delete toasts the failure with its message, and the row stays', async () => { + const ds = makeDataSource(['r1']); + renderView(ds); + await settled(); + + await userEvent.click((await openRowMenu('Alice'))!); + await userEvent.click(within(await confirmDialog()).getByRole('button', { name: 'Continue' })); + + await waitFor(() => expect(errorSpy).toHaveBeenCalledTimes(1)); + expect(ds.delete).toHaveBeenCalledWith(OBJECT, 'r1'); + expect(errorSpy).toHaveBeenCalledWith('Failed to delete Test object', { description: 'refused r1' }); + expect(successSpy).not.toHaveBeenCalled(); + expect(screen.getByText('Alice')).toBeInTheDocument(); + }); +}); + +describe('objectui#10383 — bulk Delete on the registered object-view path', () => { + it('confirms once for the selection, calls dataSource.delete per record, and the rows are gone', async () => { + const ds = makeDataSource(); + renderView(ds); + await settled(); + + selectRows(['Alice', 'Bob']); + fireEvent.click(await screen.findByTestId('bulk-action-delete')); + + const dialog = await confirmDialog(); + expect( + within(dialog).getByText('Delete 2 selected records? This cannot be undone.'), + ).toBeInTheDocument(); + expect(ds.delete).not.toHaveBeenCalled(); + + await userEvent.click(within(dialog).getByRole('button', { name: 'Continue' })); + + await waitFor(() => expect(ds.delete).toHaveBeenCalledTimes(2)); + expect(ds.delete).toHaveBeenCalledWith(OBJECT, 'r1'); + expect(ds.delete).toHaveBeenCalledWith(OBJECT, 'r2'); + expect(ds.bulkDelete).not.toHaveBeenCalled(); + await waitFor(() => expect(screen.queryByText('Alice')).not.toBeInTheDocument()); + expect(screen.queryByText('Bob')).not.toBeInTheDocument(); + expect(screen.getByText('Carol')).toBeInTheDocument(); + expect(successSpy).toHaveBeenCalledWith('Deleted 2 Test object records'); + }); + + it('a partial failure reports "N deleted, M failed" and still refreshes', async () => { + const ds = makeDataSource(['r2']); + renderView(ds); + await settled(); + + selectRows(['Alice', 'Bob']); + fireEvent.click(await screen.findByTestId('bulk-action-delete')); + await userEvent.click(within(await confirmDialog()).getByRole('button', { name: 'Continue' })); + + await waitFor(() => expect(errorSpy).toHaveBeenCalledWith('1 deleted, 1 failed')); + expect(ds.delete).toHaveBeenCalledTimes(2); + await waitFor(() => expect(screen.queryByText('Alice')).not.toBeInTheDocument()); + expect(screen.getByText('Bob')).toBeInTheDocument(); + expect(successSpy).not.toHaveBeenCalled(); + }); +}); + +describe('objectui#10383 — the affordance follows the same gate as the console list', () => { + // Selection is authored ON in both cases, so the bulk bar's reading does not + // depend on the implicit selection that a bulk Delete itself switches on. + const SELECTABLE = { table: { columns: ['name'], selection: { type: 'multiple' } } }; + + it('a principal WITH the delete grant is offered row Delete and bulk Delete (control)', async () => { + permState.noProvider = false; + permState.permDelete = true; + renderView(makeDataSource(), SELECTABLE); + await settled(); + + expect(await rowMenuEntries('Bob')).toEqual({ edit: true, delete: true }); + selectRows(['Alice']); + expect(await screen.findByTestId('bulk-action-delete')).toBeInTheDocument(); + }); + + it('a principal WITHOUT the delete grant is offered neither', async () => { + permState.noProvider = false; + permState.permDelete = false; + renderView(makeDataSource(), SELECTABLE); + await settled(); + + expect(await rowMenuEntries('Bob')).toEqual({ edit: true, delete: false }); + selectRows(['Alice']); + expect(screen.queryByTestId('bulk-action-delete')).not.toBeInTheDocument(); + }); +}); From c755610fb1780c3efc05e4ddb6547bd0c955f2b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 23:54:36 +0000 Subject: [PATCH 2/6] chore(changeset): declare the plugin-view grid Delete fix as a minor bump Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- .../10383-object-view-grid-delete-deletes.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .changeset/10383-object-view-grid-delete-deletes.md diff --git a/.changeset/10383-object-view-grid-delete-deletes.md b/.changeset/10383-object-view-grid-delete-deletes.md new file mode 100644 index 0000000000..ddf509be4a --- /dev/null +++ b/.changeset/10383-object-view-grid-delete-deletes.md @@ -0,0 +1,15 @@ +--- +'@object-ui/plugin-view': minor +--- + +The grid's row Delete and bulk Delete now delete the record when `ObjectView` renders the grid itself — the registered `object-view` renderer, with no host list view (objectui#10383). + +`ObjectGrid` hands the clicked row, or the selection, to its `onDelete` / `onBulkDelete` consumer and leaves the delete to it. `ObjectView`'s two handlers ignored the record and only refreshed, so a Delete the grid offers by default asked no question, called `dataSource.delete` zero times, and the row was still there after the refresh. + +They now behave the way the console's own object list does: + +- a row Delete asks "Are you sure you want to delete this record?"; a bulk Delete asks once for the whole selection ("Delete N selected records? This cannot be undone."); +- Continue calls `dataSource.delete(objectName, id)` for each record, then refreshes the grid, so the deleted rows are gone; Cancel deletes nothing; +- the outcome is reported with the console's toasts: "LABEL deleted successfully" / "Deleted N LABEL records", or "Failed to delete LABEL" (with the error message, and no refresh) / "N deleted, M failed". + +The dialog and toast texts reuse the translation keys the console already resolves, so they read the same in every shipped language. Whether Delete is offered at all is unchanged — it is still the grid's own verdict, the same one the console list uses: `operations.delete`, the principal's delete grant, the object's lifecycle and `userActions`, its API operations and the per-record verdict. From ea0c5c62787a7dae2045199b71404dfb097a849a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 00:15:03 +0000 Subject: [PATCH 3/6] test(plugin-view): type the grid-delete pin's casts instead of `any` Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- .../src/__tests__/ObjectView.gridDelete-10383.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx index cc9bad3713..34012dc3da 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx @@ -78,7 +78,7 @@ vi.mock('@object-ui/permissions', async (importOriginal) => { import { toast } from '@object-ui/components'; import { ActionProvider, SchemaRendererProvider } from '@object-ui/react'; -import type { DataSource } from '@object-ui/types'; +import type { DataSource, ObjectViewSchema } from '@object-ui/types'; import { ObjectView } from '../ObjectView'; import { installExplainDouble } from './explainDouble'; @@ -86,7 +86,7 @@ const OBJECT = 'test_object'; beforeAll(() => { if (!Element.prototype.scrollIntoView) { - Element.prototype.scrollIntoView = vi.fn() as any; + Element.prototype.scrollIntoView = vi.fn() as unknown as Element['scrollIntoView']; } }); @@ -150,7 +150,7 @@ function renderView(ds: ReturnType, extra: Record From 6c374d3820c0a42d8215da341bbcf1bcf6f7f39b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 00:33:56 +0000 Subject: [PATCH 4/6] refactor(core): one record-delete core that the console list and plugin-view both bind to The first repair of the plugin-view grid Delete copied the console's delete steps into a second flow, and the copy already diverged on consent: it asked a package-owned `sys_permission_set` row the plain delete question, where the console asks the ADR-0094 reset question and toasts the reset. `recordDelete` (`@object-ui/core`, the lowest package both hosts already depend on) is now the one copy: `confirmText` chooses the one-record question, and `run` performs the single delete (with the ADR-0094 detection and its `findOne` fallback) or the settled bulk delete, and reports it. The translator, toast sink, data source and refresh are injected, so core gains no i18n or toast dependency. - app-shell `useObjectActions`: the `delete` handler and `deleteRecord` call the core; the moved code is unchanged, including the `silent` / no-`error` returns that stop the runner's double toast. - plugin-view `ObjectView`: `performDelete` and the local question are gone; its AlertDialog asks the core's question and Continue runs the core, so a package-owned permission set now gets the reset copy here too. Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- .../app-shell/src/hooks/useObjectActions.ts | 130 ++--------- .../actions/__tests__/recordDelete.test.ts | 213 +++++++++++++++++ packages/core/src/actions/index.ts | 1 + packages/core/src/actions/recordDelete.ts | 215 ++++++++++++++++++ packages/plugin-view/src/ObjectView.tsx | 92 ++++---- .../ObjectView.gridDelete-10383.test.tsx | 108 +++++++++ 6 files changed, 598 insertions(+), 161 deletions(-) create mode 100644 packages/core/src/actions/__tests__/recordDelete.test.ts create mode 100644 packages/core/src/actions/recordDelete.ts diff --git a/packages/app-shell/src/hooks/useObjectActions.ts b/packages/app-shell/src/hooks/useObjectActions.ts index 6cb7585ec5..1ef16ffced 100644 --- a/packages/app-shell/src/hooks/useObjectActions.ts +++ b/packages/app-shell/src/hooks/useObjectActions.ts @@ -16,6 +16,7 @@ import { useNavigate, useParams } from 'react-router-dom'; import { useActionRunner } from '@object-ui/react'; import { useObjectTranslation } from '@object-ui/i18n'; import { toast } from 'sonner'; +import { recordDelete } from '@object-ui/core'; import type { ActionDef, ActionResult } from '@object-ui/core'; interface ObjectActionConfig { @@ -83,102 +84,21 @@ export function useObjectActions({ return { success: true }; }); - // Handler: delete - runner.registerHandler('delete', async (action: any) => { - // Accept several param shapes used across call sites: - // { params: { recordId } } — toolbar / programmatic deletes - // { params: { record } } — ObjectGrid row dropdown - // { params: { records: [...] } } — bulk delete (multi-row) - // { recordId } (legacy) — pre-params shape - const records = Array.isArray(action.params?.records) - ? action.params.records.filter((r: any) => r?.id != null) - : null; - - // Bulk path — delete every record in parallel and report a summary. - if (records && records.length > 1) { - const results = await Promise.allSettled( - records.map((r: any) => dataSource.delete(objectName, r.id)), - ); - const failed = results.filter(r => r.status === 'rejected').length; - const succeeded = results.length - failed; - onRefresh?.(); - if (failed === 0) { - toast.success( - t('objectActions.bulkDeleteSuccess', { - count: succeeded, - label: objectLabel || objectName, - defaultValue: `Deleted ${succeeded} ${objectLabel || objectName} records`, - }), - ); - // `silent`: the handler already toasted the localized summary above; - // without this the runner's post-execution hook adds a second, generic - // "Action completed successfully" toast (double success toast). - return { success: true, reload: true, silent: true }; - } - toast.error( - t('objectActions.bulkDeletePartial', { - succeeded, - failed, - defaultValue: `${succeeded} deleted, ${failed} failed`, - }), - ); - // The toast above is the authoritative feedback (it carries the - // succeeded/failed summary the runner can't reconstruct). Return - // WITHOUT `error` so the ActionRunner post-execution hook — this runner - // has a toastHandler (onToast) — doesn't fire a second, duplicate toast. - return { success: false }; - } - - const recordId = - action.params?.recordId ?? - action.params?.record?.id ?? - records?.[0]?.id ?? - action.recordId; - if (!recordId) return { success: false, error: t('objectActions.noRecordId') }; - - // [ADR-0094] "Deleting" a PACKAGE-OWNED permission set doesn't remove - // the row — the backend drops the environment overlay and RESETS the - // set to its shipped baseline. Detect it BEFORE the delete (the row - // still exists) so the success toast tells the truth; a caller that - // passed the row spares the lookup. - let packagedSetReset = false; - if (objectName === 'sys_permission_set') { - try { - const row = - action.params?.record?.managed_by !== undefined - ? action.params.record - : await dataSource.findOne(objectName, recordId); - packagedSetReset = row?.managed_by === 'package'; - } catch { /* best-effort — fall back to the generic delete copy */ } - } - - try { - await dataSource.delete(objectName, recordId); - onRefresh?.(); - toast.success( - packagedSetReset - // No `label` argument, unlike the `deleteSuccess` branch below: this - // sentence names the permission set by kind rather than by label, so - // none of the ten packs has a `{{label}}` hole and i18next dropped - // the argument in silence (objectui#3845). - ? t('objectActions.resetPackageSetSuccess', { - defaultValue: 'Permission set reset to its shipped baseline', - }) - : t('objectActions.deleteSuccess', { label: objectLabel || objectName }), - ); - // `silent`: handler owns the localized success toast above — suppress the - // runner's generic duplicate (see the bulk branch). - return { success: true, reload: true, silent: true }; - } catch (err: any) { - toast.error(t('objectActions.deleteFailed', { label: objectLabel || objectName }), { - description: err.message, - }); - // Keep the richer toast above (label + error description) and return - // WITHOUT `error` so the ActionRunner post-execution hook doesn't toast - // the raw message a second time. See the bulk branch for the rationale. - return { success: false }; - } - }); + // Handler: delete — the shared record-delete core (objectui#10383), the + // same one `plugin-view`'s grid Delete binds to. Its param shapes: + // { params: { recordId } } — toolbar / programmatic deletes + // { params: { record } } — ObjectGrid row dropdown + // { params: { records: [...] } } — bulk delete (multi-row) + // { recordId } (legacy) — pre-params shape + // It owns the toasts and returns without `error` (and `silent` on + // success), so the runner's post-execution hook adds no second toast; the + // ADR-0094 package-owned permission-set reset copy lives there too. + runner.registerHandler('delete', (action: any) => + recordDelete.run( + { objectName, label: objectLabel || objectName, dataSource, t, toast, onRefresh }, + action, + ), + ); // Handler: navigate runner.registerHandler('navigate', async (action: any) => { @@ -202,21 +122,13 @@ export function useObjectActions({ const deleteRecord = useCallback( async (recordId: string, record?: Record) => { - // [ADR-0094] A package-owned permission set is never removed by the data - // door — the backend drops the environment overlay and RESETS the set to - // its shipped baseline. Ask the honest question instead of promising an - // irreversible delete the user can see doesn't happen (the row stays). - const packagedSetReset = - objectName === 'sys_permission_set' && (record as any)?.managed_by === 'package'; + // The question comes from the shared record-delete core (objectui#10383): + // for a package-owned permission set it is the honest RESET question + // (ADR-0094), not a promise of an irreversible delete the user can see + // doesn't happen (the row stays). return execute({ type: 'delete', - confirmText: packagedSetReset - ? t('objectActions.resetPackageSetConfirm', { - defaultValue: - 'This permission set ships with an installed package and cannot be removed. ' + - 'Deleting resets it to the shipped baseline and discards your environment customization. Continue?', - }) - : t('objectActions.deleteConfirm'), + confirmText: recordDelete.confirmText({ objectName, t }, record), params: record ? { recordId, record } : { recordId }, }); }, diff --git a/packages/core/src/actions/__tests__/recordDelete.test.ts b/packages/core/src/actions/__tests__/recordDelete.test.ts new file mode 100644 index 0000000000..ed46faeaa6 --- /dev/null +++ b/packages/core/src/actions/__tests__/recordDelete.test.ts @@ -0,0 +1,213 @@ +/** + * 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 shared record-delete core (objectui#10383, ADR-0094), pinned at the unit + * level: the question, the single delete, the bulk delete, the partial failure + * and the package-owned permission-set reset — with the runner-facing result + * shape each path returns, because the console registers `run` as its action + * runner's `delete` handler and those shapes are what keep the runner from + * toasting a second time. + * + * The translator echoes the key (plus `|defaultValue` when one is passed) so + * each assertion names the copy it expects without depending on a pack. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { recordDelete } from '../recordDelete'; + +const t = (key: string, options?: Record) => { + const vars = { ...(options ?? {}) }; + delete vars.defaultValue; + const suffix = Object.keys(vars).length ? ` ${JSON.stringify(vars)}` : ''; + return `${key}${suffix}`; +}; + +function harness(opts: { + objectName?: string; + failIds?: string[]; + findOne?: (resource: string, id: string) => Promise; +} = {}) { + const failIds = opts.failIds ?? []; + const dataSource = { + delete: vi.fn(async (_resource: string, id: string) => { + if (failIds.includes(id)) throw new Error(`refused ${id}`); + return true; + }), + findOne: vi.fn(opts.findOne ?? (async () => null)), + }; + const toast = { success: vi.fn(), error: vi.fn() }; + const onRefresh = vi.fn(); + const deps = { + objectName: opts.objectName ?? 'crm_lead', + label: 'Lead', + t, + dataSource, + toast, + onRefresh, + }; + return { deps, dataSource, toast, onRefresh }; +} + +describe('recordDelete.confirmText', () => { + it('asks the plain delete question for an ordinary record', () => { + expect(recordDelete.confirmText({ objectName: 'crm_lead', t }, { id: 'l1' })).toBe( + 'objectActions.deleteConfirm', + ); + }); + + it('asks the RESET question for a package-owned permission set (ADR-0094)', () => { + expect( + recordDelete.confirmText( + { objectName: 'sys_permission_set', t }, + { id: 'ps1', managed_by: 'package' }, + ), + ).toBe('objectActions.resetPackageSetConfirm'); + }); + + it('keeps the plain question for an environment-owned permission set, or with no row', () => { + expect( + recordDelete.confirmText({ objectName: 'sys_permission_set', t }, { id: 'ps1', managed_by: 'user' }), + ).toBe('objectActions.deleteConfirm'); + expect(recordDelete.confirmText({ objectName: 'sys_permission_set', t })).toBe( + 'objectActions.deleteConfirm', + ); + }); +}); + +describe('recordDelete.run — one record', () => { + it('deletes, refreshes, toasts success, and returns the silent success the runner expects', async () => { + const { deps, dataSource, toast, onRefresh } = harness(); + const res = await recordDelete.run(deps, { params: { recordId: 'l1' } }); + + expect(dataSource.delete).toHaveBeenCalledTimes(1); + expect(dataSource.delete).toHaveBeenCalledWith('crm_lead', 'l1'); + expect(onRefresh).toHaveBeenCalledTimes(1); + expect(toast.success).toHaveBeenCalledWith('objectActions.deleteSuccess {"label":"Lead"}'); + expect(toast.error).not.toHaveBeenCalled(); + expect(res).toEqual({ success: true, reload: true, silent: true }); + }); + + it('a refused delete toasts the failure with its message, does not refresh, and returns no `error`', async () => { + const { deps, toast, onRefresh } = harness({ failIds: ['l1'] }); + const res = await recordDelete.run(deps, { params: { recordId: 'l1' } }); + + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith('objectActions.deleteFailed {"label":"Lead"}', { + description: 'refused l1', + }); + expect(onRefresh).not.toHaveBeenCalled(); + expect(res).toEqual({ success: false }); + }); + + it('reads the id off `params.record`, then a single `params.records` entry, then the legacy top-level `recordId`', async () => { + const a = harness(); + await recordDelete.run(a.deps, { params: { record: { id: 'from-record' } } }); + expect(a.dataSource.delete).toHaveBeenCalledWith('crm_lead', 'from-record'); + + const b = harness(); + await recordDelete.run(b.deps, { params: { records: [{ id: 'only-one' }] } }); + expect(b.dataSource.delete).toHaveBeenCalledWith('crm_lead', 'only-one'); + + const c = harness(); + await recordDelete.run(c.deps, { recordId: 'legacy' }); + expect(c.dataSource.delete).toHaveBeenCalledWith('crm_lead', 'legacy'); + }); + + it('with no id at all, deletes nothing and returns the `error` for the runner to report', async () => { + const { deps, dataSource, toast } = harness(); + const res = await recordDelete.run(deps, { params: {} }); + + expect(dataSource.delete).not.toHaveBeenCalled(); + expect(toast.success).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + expect(res).toEqual({ success: false, error: 'objectActions.noRecordId' }); + }); +}); + +describe('recordDelete.run — several records', () => { + it('deletes each record, refreshes once, and toasts ONE summary', async () => { + const { deps, dataSource, toast, onRefresh } = harness(); + const res = await recordDelete.run(deps, { + params: { records: [{ id: 'a' }, { id: 'b' }, { name: 'no id — skipped' }] }, + }); + + expect(dataSource.delete).toHaveBeenCalledTimes(2); + expect(dataSource.delete).toHaveBeenCalledWith('crm_lead', 'a'); + expect(dataSource.delete).toHaveBeenCalledWith('crm_lead', 'b'); + expect(onRefresh).toHaveBeenCalledTimes(1); + expect(toast.success).toHaveBeenCalledTimes(1); + expect(toast.success).toHaveBeenCalledWith( + 'objectActions.bulkDeleteSuccess {"count":2,"label":"Lead"}', + ); + expect(res).toEqual({ success: true, reload: true, silent: true }); + }); + + it('a partial failure toasts "N deleted, M failed" once, still refreshes, and returns no `error`', async () => { + const { deps, dataSource, toast, onRefresh } = harness({ failIds: ['b'] }); + const res = await recordDelete.run(deps, { params: { records: [{ id: 'a' }, { id: 'b' }] } }); + + expect(dataSource.delete).toHaveBeenCalledTimes(2); + expect(onRefresh).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith( + 'objectActions.bulkDeletePartial {"succeeded":1,"failed":1}', + ); + expect(toast.success).not.toHaveBeenCalled(); + expect(res).toEqual({ success: false }); + }); +}); + +describe('recordDelete.run — ADR-0094 package-owned permission set', () => { + it('toasts the RESET when the row passed in is package-owned, without a lookup', async () => { + const { deps, dataSource, toast } = harness({ objectName: 'sys_permission_set' }); + await recordDelete.run(deps, { + params: { recordId: 'ps1', record: { id: 'ps1', managed_by: 'package' } }, + }); + + expect(dataSource.delete).toHaveBeenCalledWith('sys_permission_set', 'ps1'); + expect(dataSource.findOne).not.toHaveBeenCalled(); + expect(toast.success).toHaveBeenCalledWith('objectActions.resetPackageSetSuccess'); + }); + + it('looks the row up with findOne when only the id is known, and toasts the reset', async () => { + const { deps, dataSource, toast } = harness({ + objectName: 'sys_permission_set', + findOne: async () => ({ id: 'ps1', managed_by: 'package' }), + }); + await recordDelete.run(deps, { params: { recordId: 'ps1' } }); + + expect(dataSource.findOne).toHaveBeenCalledWith('sys_permission_set', 'ps1'); + expect(toast.success).toHaveBeenCalledWith('objectActions.resetPackageSetSuccess'); + }); + + it('an environment-owned set, or a failed lookup, keeps the plain delete copy (control)', async () => { + const env = harness({ objectName: 'sys_permission_set' }); + await recordDelete.run(env.deps, { + params: { recordId: 'ps1', record: { id: 'ps1', managed_by: 'user' } }, + }); + expect(env.toast.success).toHaveBeenCalledWith('objectActions.deleteSuccess {"label":"Lead"}'); + + const lost = harness({ + objectName: 'sys_permission_set', + findOne: async () => { + throw new Error('lookup failed'); + }, + }); + await recordDelete.run(lost.deps, { params: { recordId: 'ps1' } }); + // Best-effort: the lookup failing does not block the delete. + expect(lost.dataSource.delete).toHaveBeenCalledWith('sys_permission_set', 'ps1'); + expect(lost.toast.success).toHaveBeenCalledWith('objectActions.deleteSuccess {"label":"Lead"}'); + }); + + it('an ordinary object is never looked up', async () => { + const { deps, dataSource } = harness(); + await recordDelete.run(deps, { params: { recordId: 'l1' } }); + expect(dataSource.findOne).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/actions/index.ts b/packages/core/src/actions/index.ts index 1673919055..f286ff5512 100644 --- a/packages/core/src/actions/index.ts +++ b/packages/core/src/actions/index.ts @@ -16,3 +16,4 @@ export * from './actionErrorDetail.js'; export * from './actionResponse.js'; export * from './serverActionHandler.js'; export * from './recordIdParam.js'; +export * from './recordDelete.js'; diff --git a/packages/core/src/actions/recordDelete.ts b/packages/core/src/actions/recordDelete.ts new file mode 100644 index 0000000000..bc25ece015 --- /dev/null +++ b/packages/core/src/actions/recordDelete.ts @@ -0,0 +1,215 @@ +/** + * 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 ONE record-delete core — the confirm question and the delete itself — + * that every host's list Delete binds to (objectui#10383). + * + * ## Why this lives here, and why there is only one + * + * The console's object list deleted through `useObjectActions` (`app-shell`): + * the confirm question, the single delete with its ADR-0094 detection, the bulk + * delete and their toasts were written inline in that hook. The registered + * `object-view` renderer (`plugin-view`) had no delete at all — its grid + * handlers only refreshed — and the first repair COPIED those steps into a + * second flow. The copy already diverged on consent: it asked every row the + * plain "delete this record?" question, while the console asks a package-owned + * permission set the honest reset question. One operation with two + * implementations drifts on exactly the part that matters, so the rule is: the + * side with the governance keeps it, the other side re-binds to it, and there + * is one copy. This module is that copy, and both hosts call it. + * + * `@object-ui/core` is the lowest package both hosts already depend on. The + * host-specific pieces are INJECTED — the translator, the toast sink, the data + * source and the refresh — so this package gains no i18n or toast dependency. + * The host keeps its own confirm UI (`app-shell` runs the question through the + * action runner's confirm handler; `plugin-view` renders it in its own + * AlertDialog); the QUESTION comes from {@link recordDelete}.confirmText. + * + * ## ADR-0094 — a package-owned permission set is RESET, not deleted + * + * "Deleting" a `sys_permission_set` row whose `managed_by` is `'package'` does + * not remove it: the backend drops the environment overlay and resets the set + * to its shipped baseline. So the confirm asks the reset question and the + * success toast says "reset" — a promise of an irreversible delete the user can + * then see did not happen would be the dishonest copy. The row passed in is + * read first; when the caller has only an id (an SDUI header delete), the row + * is looked up with `findOne`, best-effort — a failed lookup falls back to the + * generic copy rather than blocking the delete. + * + * ## The result shape is the action runner's, on purpose + * + * `run` returns what a registered `delete` handler returns, because the console + * registers it as exactly that. The toasts here are the AUTHORITATIVE feedback, + * so every path returns WITHOUT an `error` key (and success returns `silent`) — + * otherwise the runner's post-execution hook, which has its own toast handler, + * would toast the same outcome a second time. The single exception is a + * request with no record id at all, which toasts nothing here and returns the + * `error` for the runner to report. + * + * Record ids are forwarded exactly as the caller supplied them; nothing here + * converts them. + */ + +import type { ActionResult } from './ActionRunner.js'; + +type Row = Record; + +/** Translate a key — the host's `t`, injected so core takes no i18n dependency. */ +type Translate = (key: string, options?: Record) => string; + +/** Everything `run` needs from its host. `confirmText` needs only the first two. */ +interface RecordDeleteDeps { + objectName: string; + t: Translate; + /** The object's display label, used by the toasts. */ + label: string; + dataSource: { + delete(resource: string, id: string): Promise; + findOne(resource: string, id: string): Promise; + }; + /** The host's toast sink (sonner's `toast` in both hosts today). */ + toast: { + success(message: string): unknown; + error(message: string, data?: { description?: string }): unknown; + }; + /** Re-read the list after a delete. */ + onRefresh?: () => void; +} + +/** + * The request, in the action runner's `delete` shape — the console registers + * `run` as its `delete` handler, so the runner's action arrives here verbatim: + * `params.records` (several), `params.recordId` + optional `params.record` + * (one), and the legacy top-level `recordId`. + */ +interface RecordDeleteRequest { + params?: { records?: unknown; recordId?: unknown; record?: Row | null }; + recordId?: unknown; +} + +const PERMISSION_SET_OBJECT = 'sys_permission_set'; + +/** ADR-0094: is this permission-set row owned by an installed package? */ +function isPackageOwned(row: unknown): boolean { + return (row as Row | null | undefined)?.managed_by === 'package'; +} + +/** + * The record-delete core: `confirmText` is the question to ask before deleting + * one record, `run` performs the delete (one record, or several settled + * together) and reports it. See the module docblock for objectui#10383 and + * ADR-0094. + */ +export const recordDelete = Object.freeze({ + /** + * The question to ask before deleting ONE record: the reset question for a + * package-owned permission set (ADR-0094), the plain delete question + * otherwise. Reads only the row it is handed — no lookup. + */ + confirmText(deps: Pick, record?: Row | null): string { + const { objectName, t } = deps; + return objectName === PERMISSION_SET_OBJECT && isPackageOwned(record) + ? t('objectActions.resetPackageSetConfirm', { + defaultValue: + 'This permission set ships with an installed package and cannot be removed. ' + + 'Deleting resets it to the shipped baseline and discards your environment customization. Continue?', + }) + : t('objectActions.deleteConfirm'); + }, + + /** + * Delete, refresh and report. Several records (`params.records`, more than + * one with an `id`) are deleted one call each and settled together, then the + * list refreshes and ONE summary toast reports the batch. Otherwise one + * record is deleted; on success the list refreshes and a success toast shows + * (the reset copy for a package-owned permission set), on failure an error + * toast carries the error message and the list is left as it is. + */ + async run(deps: RecordDeleteDeps, request: RecordDeleteRequest): Promise { + const { objectName, t, label, dataSource, toast, onRefresh } = deps; + const params = request.params; + const records = Array.isArray(params?.records) + ? (params.records as Row[]).filter((r) => r?.id != null) + : null; + + // Bulk path — delete every record in parallel and report a summary. + if (records && records.length > 1) { + const results = await Promise.allSettled( + records.map((r) => dataSource.delete(objectName, r.id as string)), + ); + const failed = results.filter((r) => r.status === 'rejected').length; + const succeeded = results.length - failed; + onRefresh?.(); + if (failed === 0) { + toast.success( + t('objectActions.bulkDeleteSuccess', { + count: succeeded, + label, + defaultValue: `Deleted ${succeeded} ${label} records`, + }), + ); + // `silent`: the summary above is the feedback — without it the runner's + // post-execution hook adds a second, generic success toast. + return { success: true, reload: true, silent: true }; + } + toast.error( + t('objectActions.bulkDeletePartial', { + succeeded, + failed, + defaultValue: `${succeeded} deleted, ${failed} failed`, + }), + ); + // The summary above is authoritative; returning WITHOUT `error` keeps the + // runner from toasting the failure a second time. + return { success: false }; + } + + const recordId = + params?.recordId ?? params?.record?.id ?? records?.[0]?.id ?? request.recordId; + if (!recordId) return { success: false, error: t('objectActions.noRecordId') }; + + // [ADR-0094] Detect the reset BEFORE the delete (the row still exists), so + // the success toast tells the truth. A caller that passed the row spares + // the lookup. + let packagedSetReset = false; + if (objectName === PERMISSION_SET_OBJECT) { + try { + const row = + params?.record?.managed_by !== undefined + ? params.record + : await dataSource.findOne(objectName, recordId as string); + packagedSetReset = isPackageOwned(row); + } catch { /* best-effort — fall back to the generic delete copy */ } + } + + try { + await dataSource.delete(objectName, recordId as string); + onRefresh?.(); + toast.success( + packagedSetReset + // No `label` argument, unlike `deleteSuccess` below: this sentence + // names the permission set by kind rather than by label, so none of + // the ten packs has a `{{label}}` hole and i18next would drop the + // argument in silence (objectui#3845). + ? t('objectActions.resetPackageSetSuccess', { + defaultValue: 'Permission set reset to its shipped baseline', + }) + : t('objectActions.deleteSuccess', { label }), + ); + return { success: true, reload: true, silent: true }; + } catch (err) { + toast.error(t('objectActions.deleteFailed', { label }), { + description: (err as Error).message, + }); + // The toast above (label + error message) is the feedback; returning + // WITHOUT `error` keeps the runner from toasting the raw message again. + return { success: false }; + } + }, +}); diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index f011d9f832..a8eb765bf7 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -73,6 +73,7 @@ import { mergeFilterNodes, columnIdentity, convertSortToQueryParams, + recordDelete, } from '@object-ui/core'; import { SchemaRenderer as ImportedSchemaRenderer, useSettledSchema } from '@object-ui/react'; import type { HandleClickModifiers } from '@object-ui/react'; @@ -298,11 +299,13 @@ const VIEW_DEFAULT_TRANSLATIONS: Record = { 'form.editTitle': 'Edit {{object}}', 'form.viewTitle': 'View {{object}}', // objectui#10383 — the grid's row / bulk Delete. Not new keys: these are the - // ones the console's own list delete resolves (`useObjectActions` for the - // confirm text and the toasts, the console `ObjectView` for the bulk confirm, - // `ActionConfirmDialog` for the dialog chrome), all present in the ten packs. - // Reusing them is what keeps this path's dialog and toasts word-for-word the - // console list's, in every language. + // ones the console's own list delete resolves, all present in the ten packs. + // The `objectActions.*` rows are asked for by the shared `recordDelete` core + // (`@object-ui/core`), which this view hands `tView`, so a provider-less host + // reads them here; the ADR-0094 reset rows carry their own inline + // `defaultValue` there. `console.objectView.bulkDeleteConfirm` and the + // `actionConfirm.*` chrome are this host's dialog, in the console's + // `ActionConfirmDialog` shape. 'actionConfirm.title': 'Confirm Action', 'actionConfirm.confirm': 'Continue', 'actionConfirm.cancel': 'Cancel', @@ -1366,16 +1369,16 @@ export const ObjectView: React.FC = ({ // this path — the registered `object-view` renderer, no host list view — a // Delete offered by default deleted nothing and the row came back. // - // They now do what the console's own list does with the same two callbacks - // (`app-shell` `ObjectView` → `useObjectActions`), step for step: - // - a row asks `objectActions.deleteConfirm`; a selection asks - // `console.objectView.bulkDeleteConfirm` once for the whole batch; - // - a row without an `id` is skipped, as the console skips it; - // - one record: `dataSource.delete`, then refresh + `deleteSuccess`, or the - // `deleteFailed` toast with the error's message and NO refresh; - // - several: `dataSource.delete` per record, settled together (the console - // does not use the optional `bulkDelete`), then refresh, then - // `bulkDeleteSuccess` or `bulkDeletePartial`. + // They now bind to the SAME record-delete core the console's own list binds + // to (`recordDelete` in `@object-ui/core`, which `app-shell`'s + // `useObjectActions` registers as its `delete` handler). This host owns only + // its confirm UI (the AlertDialog below) and the bulk question; the one-row + // question — including ADR-0094's reset question for a package-owned + // permission set — the delete, the toasts and when to refresh all come from + // that core, so the two paths cannot drift. The requests are shaped the way + // the console's are: a row as `{ recordId, record }`, a selection as + // `{ records }`, rows without an `id` skipped first. + // // The permission half needs nothing here: whether the Delete affordance is // offered at all is `ObjectGrid`'s verdict on both paths (`operations`, the // principal's `can(object, 'delete')`, the object's bucket / `userActions` / @@ -1398,37 +1401,6 @@ export const ObjectView: React.FC = ({ setDeleteRequest({ open: true, records: valid, bulk: true }); }, []); - const performDelete = async (records: Record[]) => { - const objectName = schema.objectName; - const label = (objectSchema?.label as string) || objectName; - if (records.length > 1) { - // `async` callback on purpose: a throw inside it becomes a rejected - // settlement and is counted as a failure, instead of escaping `map` - // before `allSettled` ever sees the batch. - const results = await Promise.allSettled( - records.map(async (r) => dataSource.delete(objectName, String(r.id))), - ); - const failed = results.filter((r) => r.status === 'rejected').length; - const succeeded = results.length - failed; - setRefreshKey(prev => prev + 1); - if (failed === 0) { - toast.success(tView('objectActions.bulkDeleteSuccess', { count: succeeded, label })); - } else { - toast.error(tView('objectActions.bulkDeletePartial', { succeeded, failed })); - } - return; - } - try { - await dataSource.delete(objectName, String(records[0].id)); - setRefreshKey(prev => prev + 1); - toast.success(tView('objectActions.deleteSuccess', { label })); - } catch (err) { - toast.error(tView('objectActions.deleteFailed', { label }), { - description: (err as Error | undefined)?.message, - }); - } - }; - // Handle form submission const handleFormSuccess = useCallback(() => { setIsFormOpen(false); @@ -2595,9 +2567,10 @@ export const ObjectView: React.FC = ({ // empty `mb-4` spacer div above the content. const toolbar = renderToolbar(); - // The delete confirmation (objectui#10383) — the console's - // `ActionConfirmDialog` shape and keys: `actionConfirm.*` chrome, the - // question as the description. Close flips `open` and KEEPS the request, so + // The delete confirmation (objectui#10383) — this host's confirm UI, in the + // console's `ActionConfirmDialog` shape and keys (`actionConfirm.*` chrome, + // the question as the description). The one-row question and the delete + // itself come from the shared `recordDelete` core. Close flips `open` and KEEPS the request, so // the description does not blank during the exit animation (the objectui#6034 // lesson); the `open` guard on Continue is what stops a click during that // animation from deleting twice. @@ -2614,7 +2587,10 @@ export const ObjectView: React.FC = ({ {deleteRequest?.bulk ? tView('console.objectView.bulkDeleteConfirm', { count: deleteRequest.records.length }) - : tView('objectActions.deleteConfirm')} + : recordDelete.confirmText( + { objectName: schema.objectName, t: tView }, + deleteRequest?.records[0], + )} @@ -2622,9 +2598,21 @@ export const ObjectView: React.FC = ({ { if (!deleteRequest?.open) return; - const { records } = deleteRequest; + const { records, bulk } = deleteRequest; setDeleteRequest({ ...deleteRequest, open: false }); - void performDelete(records); + void recordDelete.run( + { + objectName: schema.objectName, + label: (objectSchema?.label as string) || schema.objectName, + dataSource, + t: tView, + toast, + onRefresh: () => setRefreshKey(prev => prev + 1), + }, + bulk + ? { params: { records } } + : { params: { recordId: String(records[0].id), record: records[0] } }, + ); }} > {tView('actionConfirm.confirm')} diff --git a/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx index 34012dc3da..0fbb36d087 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.gridDelete-10383.test.tsx @@ -35,6 +35,11 @@ * the same one the console list rides: a principal without the delete grant * sees neither the row Delete nor the bulk Delete on this path. * + * Those steps are not re-implemented on this path: both hosts bind to ONE + * record-delete core (`recordDelete`, `@object-ui/core`). The last block pins + * the part a copy got wrong — ADR-0094's reset question and toast for a + * package-owned permission set. + * * Every case drives the real `ObjectGrid` through `ObjectView`, the way the * registered renderer composes them, against an in-memory data source whose * `delete` really removes the row — so "the row is gone" is read off the @@ -333,3 +338,106 @@ describe('objectui#10383 — the affordance follows the same gate as the console expect(screen.queryByTestId('bulk-action-delete')).not.toBeInTheDocument(); }); }); + +/** + * ADR-0094 on this path — the reason the delete core is SHARED rather than + * copied. "Deleting" a package-owned `sys_permission_set` row resets it to its + * shipped baseline, so the console asks the reset question and toasts "reset". + * A copied flow asked this row the plain delete question; bound to the shared + * `recordDelete` core, this path asks what the console asks. + */ +describe('objectui#10383 — ADR-0094 reset copy on the registered object-view path', () => { + const PERMISSION_SET = 'sys_permission_set'; + + function makePermissionSetSource(managedBy: string) { + const ds = { + find: vi.fn(async () => ({ + data: [{ id: 'ps1', name: 'Sales Rep', managed_by: managedBy }], + total: 1, + })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(async () => true), + getObjectSchema: vi.fn(async () => ({ + name: PERMISSION_SET, + label: 'Permission Set', + fields: { + id: { type: 'text' }, + name: { type: 'text', label: 'Name' }, + managed_by: { type: 'text', label: 'Managed by' }, + }, + })), + }; + return ds; + } + + function renderPermissionSets(ds: ReturnType) { + return render( + + + + + , + ); + } + + async function deleteSalesRep() { + await waitFor(() => expect(screen.getByText('Sales Rep')).toBeInTheDocument()); + const deleteItem = await (async () => { + const row = screen.getByText('Sales Rep').closest('tr'); + await waitFor(() => + expect(row?.querySelector('[data-testid="row-action-trigger"]')).not.toBeNull(), + ); + return openRowMenu('Sales Rep'); + })(); + expect(deleteItem, 'the row kebab offers no Delete — the harness is not reaching the subject').not.toBeNull(); + await userEvent.click(deleteItem!); + return confirmDialog(); + } + + it('a package-owned permission set asks the RESET question and toasts the reset', async () => { + const ds = makePermissionSetSource('package'); + renderPermissionSets(ds); + + const dialog = await deleteSalesRep(); + expect( + within(dialog).getByText( + 'This permission set ships with an installed package and cannot be removed. ' + + 'Deleting resets it to the shipped baseline and discards your environment customization. Continue?', + ), + ).toBeInTheDocument(); + + await userEvent.click(within(dialog).getByRole('button', { name: 'Continue' })); + + await waitFor(() => expect(ds.delete).toHaveBeenCalledWith(PERMISSION_SET, 'ps1')); + await waitFor(() => + expect(successSpy).toHaveBeenCalledWith('Permission set reset to its shipped baseline'), + ); + // The row itself answered the ownership question — no lookup was needed. + expect(ds.findOne).not.toHaveBeenCalled(); + }); + + it('an environment-owned permission set keeps the plain delete copy (control)', async () => { + const ds = makePermissionSetSource('user'); + renderPermissionSets(ds); + + const dialog = await deleteSalesRep(); + expect( + within(dialog).getByText('Are you sure you want to delete this record?'), + ).toBeInTheDocument(); + + await userEvent.click(within(dialog).getByRole('button', { name: 'Continue' })); + + await waitFor(() => + expect(successSpy).toHaveBeenCalledWith('Permission Set deleted successfully'), + ); + }); +}); From e9668789f1b502776e7cd5c5405f52cff2d60daf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 00:52:10 +0000 Subject: [PATCH 5/6] test(scripts): re-point the objectui#3845 call-site pin to the record-delete core `resetPackageSetSuccess` moved, unchanged, from app-shell's `useObjectActions.ts` into `@object-ui/core`'s `recordDelete`, so the pin that proves its inert `label` argument stays deleted now reads that file, and it also pins the sister `deleteSuccess` call that still passes its `label`. The core import in `useObjectActions.ts` is folded into the existing `@object-ui/core` import line, so no line below it moves; that keeps the `useObjectActions.ts:21` citation in `one-authority-per-exported-name-6273.test.ts` true. Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- packages/app-shell/src/hooks/useObjectActions.ts | 3 +-- .../__tests__/check-i18n-call-site-keys.test.ts | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/app-shell/src/hooks/useObjectActions.ts b/packages/app-shell/src/hooks/useObjectActions.ts index 1ef16ffced..82932b2bfc 100644 --- a/packages/app-shell/src/hooks/useObjectActions.ts +++ b/packages/app-shell/src/hooks/useObjectActions.ts @@ -16,8 +16,7 @@ import { useNavigate, useParams } from 'react-router-dom'; import { useActionRunner } from '@object-ui/react'; import { useObjectTranslation } from '@object-ui/i18n'; import { toast } from 'sonner'; -import { recordDelete } from '@object-ui/core'; -import type { ActionDef, ActionResult } from '@object-ui/core'; +import { recordDelete, type ActionDef, type ActionResult } from '@object-ui/core'; interface ObjectActionConfig { objectName: string; diff --git a/scripts/__tests__/check-i18n-call-site-keys.test.ts b/scripts/__tests__/check-i18n-call-site-keys.test.ts index 54fa0dbb44..d2183b6d56 100644 --- a/scripts/__tests__/check-i18n-call-site-keys.test.ts +++ b/scripts/__tests__/check-i18n-call-site-keys.test.ts @@ -898,8 +898,12 @@ export const A = () => { const { t } = useObjectTranslation(); return t('auth.fo "t('home.welcome', { defaultValue: 'Build your business system with AI' })", ], [ - 'packages/app-shell/src/hooks/useObjectActions.ts', - "t('objectActions.resetPackageSetSuccess', {\n label:", + // objectui#10383 moved this call, unchanged, out of `app-shell`'s + // `useObjectActions.ts` into the one record-delete core both hosts now + // bind to. Same call, same argument list; the indentation is the new + // file's. + 'packages/core/src/actions/recordDelete.ts', + "t('objectActions.resetPackageSetSuccess', {\n label:", "t('objectActions.resetPackageSetSuccess', {", ], ]; @@ -915,6 +919,14 @@ export const A = () => { const { t } = useObjectTranslation(); return t('auth.fo 'utf8', ); expect(marketplace).toContain("t('marketplace.install.updateTo', { defaultValue: 'Update \\u2192 v{{version}}', version: latestVersion })"); + // Same guard for the record-delete core: `resetPackageSetSuccess` lost its + // inert `label`, and the sister `deleteSuccess` beside it — whose en value + // HAS a `{{label}}` hole — still passes it. + const recordDeleteCore = fs.readFileSync( + path.join(repoRoot, 'packages/core/src/actions/recordDelete.ts'), + 'utf8', + ); + expect(recordDeleteCore).toContain("t('objectActions.deleteSuccess', { label })"); }); }); From 473c86933be38cf4c768aff52acf6d94df51c544 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 01:22:54 +0000 Subject: [PATCH 6/6] chore(changeset): declare the record-delete core (core minor) and restate the plugin-view entry `@object-ui/core` gains the `recordDelete` export, so it gets a minor bump. The plugin-view entry now says the grid Delete binds to that shared core, so a package-owned permission set gets the reset question and toast on this path too. No app-shell changeset: the console's delete behaviour is unchanged, and check-changeset-presence does not require one. Claude-Session: https://claude.ai/code/session_01BA3nKVUwKQJf8DBxrSVtNC --- .changeset/10383-object-view-grid-delete-deletes.md | 10 +++++----- .changeset/10383-record-delete-core.md | 11 +++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 .changeset/10383-record-delete-core.md diff --git a/.changeset/10383-object-view-grid-delete-deletes.md b/.changeset/10383-object-view-grid-delete-deletes.md index ddf509be4a..e818ed6768 100644 --- a/.changeset/10383-object-view-grid-delete-deletes.md +++ b/.changeset/10383-object-view-grid-delete-deletes.md @@ -6,10 +6,10 @@ The grid's row Delete and bulk Delete now delete the record when `ObjectView` re `ObjectGrid` hands the clicked row, or the selection, to its `onDelete` / `onBulkDelete` consumer and leaves the delete to it. `ObjectView`'s two handlers ignored the record and only refreshed, so a Delete the grid offers by default asked no question, called `dataSource.delete` zero times, and the row was still there after the refresh. -They now behave the way the console's own object list does: +They now bind to the same record-delete core as the console's own object list (`recordDelete` from `@object-ui/core`), so the two paths behave the same: -- a row Delete asks "Are you sure you want to delete this record?"; a bulk Delete asks once for the whole selection ("Delete N selected records? This cannot be undone."); -- Continue calls `dataSource.delete(objectName, id)` for each record, then refreshes the grid, so the deleted rows are gone; Cancel deletes nothing; -- the outcome is reported with the console's toasts: "LABEL deleted successfully" / "Deleted N LABEL records", or "Failed to delete LABEL" (with the error message, and no refresh) / "N deleted, M failed". +- a row Delete asks "Are you sure you want to delete this record?". A package-owned permission set (a `sys_permission_set` row whose `managed_by` is `'package'`) is asked the reset question instead, because deleting it resets it to its shipped baseline rather than removing it. A bulk Delete asks once for the whole selection ("Delete N selected records? This cannot be undone."); +- Continue calls `dataSource.delete(objectName, id)` for each record, then refreshes the grid, so the deleted rows are gone. Cancel deletes nothing; +- the outcome is reported with the console's toasts: "LABEL deleted successfully" (or the reset message for a package-owned permission set) / "Deleted N LABEL records", or "Failed to delete LABEL" (with the error message, and no refresh) / "N deleted, M failed". -The dialog and toast texts reuse the translation keys the console already resolves, so they read the same in every shipped language. Whether Delete is offered at all is unchanged — it is still the grid's own verdict, the same one the console list uses: `operations.delete`, the principal's delete grant, the object's lifecycle and `userActions`, its API operations and the per-record verdict. +The confirm dialog is `ObjectView`'s own, in the console's dialog shape. Its texts reuse translation keys the console already resolves, so they read the same in every shipped language. Whether Delete is offered at all is unchanged. It is still the grid's own verdict, the same one the console list uses: `operations.delete`, the principal's delete grant, the object's lifecycle and `userActions`, its API operations and the per-record verdict. diff --git a/.changeset/10383-record-delete-core.md b/.changeset/10383-record-delete-core.md new file mode 100644 index 0000000000..9b30d9c771 --- /dev/null +++ b/.changeset/10383-record-delete-core.md @@ -0,0 +1,11 @@ +--- +'@object-ui/core': minor +--- + +New export `recordDelete` — the one record-delete core that list hosts bind to, so a Delete behaves the same wherever it is offered (objectui#10383). + +`recordDelete.confirmText({ objectName, t }, record?)` returns the question to ask before deleting one record. `recordDelete.run(deps, request)` performs the delete and reports it: one record, or several deleted one call each and settled together, then a refresh and the success, failure or "N deleted, M failed" toast. `deps` carries the host's `objectName`, `label`, `t`, `toast`, `dataSource` and optional `onRefresh`, so `@object-ui/core` takes no i18n or toast dependency. `request` is the action runner's `delete` shape (`params.records`, or `params.recordId` with an optional `params.record`), and the result is what a runner `delete` handler returns: the toasts are the feedback, so no path returns an `error` except a request with no record id. + +ADR-0094 lives here: a `sys_permission_set` row whose `managed_by` is `'package'` is reset to its shipped baseline, not removed, so it is asked the reset question and gets the reset toast. The row passed in is read first, with a best-effort `findOne` when only the id is known. + +The console's object list (`@object-ui/app-shell` `useObjectActions`) and the registered `object-view` grid (`@object-ui/plugin-view`) both call it. The console's behaviour is unchanged: its handler code moved here with no behaviour change, including the returns that keep the runner from showing a second toast.