fix(app-shell): stop double-firing action error/success toasts in RecordDetailView & delete handler

.changeset/dedupe-recorddetail-delete-toast.md

@@ -0,0 +1,26 @@
+---
+'@object-ui/app-shell': patch
+---
+
+Stop double-firing action toasts on record-detail script actions and the delete handler.
+
+`ActionRunner.handlePostExecution` already surfaces a result's `error` as a toast
+(and a success toast unless `silent`). Two handlers ALSO toasted themselves while
+returning `{success:false, error}` (or a non-`silent` success), so on a runner
+seeded with `onToast` the same message fired twice:
+
+- **`RecordDetailView` `serverActionHandler`** (script actions): the HTTP/inner-fail
+  branch and the catch branch each called `toast.error` before returning the error.
+  #2177 fixed the twin in `useConsoleActionRuntime` (interface pages) but not this
+  copy, so record-detail script-action failures (e.g. a `RECORD_LOCKED` from an
+  approval-locked record) still showed the error twice for everyone on the published
+  console bundle. Both branches now return the error and let the runner toast it once.
+
+- **`useObjectActions` `delete` handler** (ObjectView list/detail deletes): kept its
+  richer localized toast (label + description, or the bulk succeeded/failed summary)
+  and now returns WITHOUT `error` on failure so the runner doesn't re-toast it, and
+  marks successful deletes `silent` so the runner doesn't append a second generic
+  "Action completed successfully" toast.
+
+Adds `useObjectActions.test.tsx` asserting exactly one toast on delete
+success / failure / partial-bulk-failure.

packages/app-shell/src/hooks/__tests__/useObjectActions.test.tsx

@@ -0,0 +1,141 @@
+/**
+ * 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.
+ */
+
+/**
+ * Regression coverage for the delete handler's error feedback (double-toast fix).
+ *
+ * useObjectActions runs its handlers on an ActionRunner seeded with `onToast`
+ * (ObjectView passes its console toastHandler). The delete handler surfaces
+ * failures with its own contextual `toast.error` (label + description, or the
+ * bulk succeeded/failed summary). It must therefore return WITHOUT an `error`
+ * key — otherwise ActionRunner.handlePostExecution toasts the error a SECOND
+ * time and the user sees the same delete failure twice.
+ *
+ * These tests wire `onToast` to the same sonner sink the handler uses (exactly
+ * as ObjectView's real toastHandler does) and assert the user sees ONE toast.
+ */
+
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+import { renderHook, act } from '@testing-library/react';
+
+const navigateSpy = vi.fn();
+vi.mock('react-router-dom', () => ({
+  useNavigate: () => navigateSpy,
+  useParams: () => ({ appName: 'crm' }),
+}));
+
+vi.mock('@object-ui/i18n', () => ({
+  useObjectTranslation: () => ({
+    // Echo the interpolated defaultValue so assertions read naturally; fall
+    // back to the key when a test doesn't provide one.
+    t: (key: string, opts?: Record<string, any>) => (opts?.defaultValue ?? key),
+  }),
+}));
+
+// Spy on the toast sink. Both the handler's direct call AND the runner's
+// onToast bridge funnel here in the real app, so the call count is exactly
+// what the user sees.
+vi.mock('sonner', () => {
+  const fn: any = vi.fn();
+  fn.error = vi.fn();
+  fn.success = vi.fn();
+  return { toast: fn };
+});
+
+import { toast } from 'sonner';
+import { useObjectActions } from '../useObjectActions';
+
+// Mirror ObjectView's real toastHandler: route the runner's post-execution
+// toast into the same sonner sink the handler uses directly.
+const onToast = (message: string, options?: { type?: string }) => {
+  if (options?.type === 'error') (toast as any).error(message);
+  else (toast as any).success(message);
+};
+
+const onConfirm = async () => true;
+
+beforeEach(() => {
+  navigateSpy.mockReset();
+  (toast as any).mockClear?.();
+  (toast as any).error.mockClear();
+  (toast as any).success.mockClear();
+});
+
+function setup(dataSource: any) {
+  return renderHook(() =>
+    useObjectActions({
+      objectName: 'mtc_lead',
+      objectLabel: '线索',
+      dataSource,
+      onConfirm,
+      onToast,
+    }),
+  );
+}
+
+describe('useObjectActions — delete handler toast de-duplication', () => {
+  it('a failed single delete surfaces EXACTLY ONE error toast (not two)', async () => {
+    const dataSource = {
+      delete: vi.fn().mockRejectedValue(
+        new Error('RECORD_LOCKED: record is locked while an approval is in progress'),
+      ),
+    };
+    const { result } = setup(dataSource);
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.deleteRecord('lead-1');
+    });
+
+    // The handler owns the (richer) failure toast; the runner must stay quiet.
+    expect((toast as any).error).toHaveBeenCalledTimes(1);
+    // And it's the contextual "deleteFailed" toast (with the error as its
+    // description), not the runner's bare error string — i18n is stubbed here,
+    // so the key stands in for the interpolated label.
+    expect((toast as any).error).toHaveBeenCalledWith(
+      'objectActions.deleteFailed',
+      expect.objectContaining({ description: expect.stringContaining('RECORD_LOCKED') }),
+    );
+    // Returning without `error` is exactly what suppresses the runner's toast.
+    expect(res).toEqual({ success: false });
+  });
+
+  it('a successful delete surfaces one success toast and no error toast', async () => {
+    const dataSource = { delete: vi.fn().mockResolvedValue(undefined) };
+    const { result } = setup(dataSource);
+
+    await act(async () => {
+      await result.current.deleteRecord('lead-1');
+    });
+
+    expect((toast as any).success).toHaveBeenCalledTimes(1);
+    expect((toast as any).error).not.toHaveBeenCalled();
+  });
+
+  it('a partial bulk delete surfaces EXACTLY ONE error toast carrying the summary', async () => {
+    // One id succeeds, one rejects → "1 deleted, 1 failed" summary toast only.
+    const dataSource = {
+      delete: vi
+        .fn()
+        .mockResolvedValueOnce(undefined)
+        .mockRejectedValueOnce(new Error('boom')),
+    };
+    const { result } = setup(dataSource);
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.execute({
+        type: 'delete',
+        params: { records: [{ id: 'a' }, { id: 'b' }] },
+      } as any);
+    });
+
+    expect((toast as any).error).toHaveBeenCalledTimes(1);
+    expect(res).toEqual({ success: false });
+  });
+});

packages/app-shell/src/hooks/useObjectActions.ts

@@ -106,7 +106,10 @@ export function useObjectActions({
               defaultValue: `Deleted ${succeeded} ${objectLabel || objectName} records`,
             }),
           );
-          return { success: true, reload: true };
+          // `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', {
@@ -115,7 +118,11 @@ export function useObjectActions({
             defaultValue: `${succeeded} deleted, ${failed} failed`,
           }),
         );
-        return { success: false, error: `${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 =
@@ -129,12 +136,17 @@ export function useObjectActions({
         await dataSource.delete(objectName, recordId);
         onRefresh?.();
         toast.success(t('objectActions.deleteSuccess', { label: objectLabel || objectName }));
-        return { success: true, reload: true };
+        // `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,
         });
-        return { success: false, error: 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 };
       }
     });
 

packages/app-shell/src/views/RecordDetailView.tsx

@@ -618,9 +618,11 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
       if (!res.ok || (json && json.success === false) || innerFailed) {
         const errMsg = (innerFailed && inner.error) || json?.error || `Action "${targetName}" failed (HTTP ${res.status})`;
         if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } }
-        // Surface the failure — this custom new-tab path bypasses
-        // ActionRunner's toast-on-error, so otherwise the user gets no feedback.
-        toast.error(errMsg);
+        // Don't toast here. This handler always runs through the ActionRunner
+        // (registered as the `script` handler on the ActionProvider below, which
+        // wires `onToast`), whose post-execution hook surfaces the returned
+        // `error` as one toast. Toasting again double-fired the message
+        // (e.g. RECORD_LOCKED appeared twice). Mirrors useConsoleActionRuntime.
         return { success: false, error: errMsg };
       }
       const shouldRefresh = action.refreshAfter !== false;
@@ -663,7 +665,8 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
     } catch (error) {
       if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } }
       const msg = (error as Error).message;
-      toast.error(msg);
+      // Don't toast here — the ActionRunner post-execution hook toasts the
+      // returned `error` once (see the failure branch above).
       return { success: false, error: msg };
     } finally {
       serverActionInFlight.current.delete(inflightKey);