From 74ed372080245a846b8aa2080d46ca430ee48609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sun, 5 Jul 2026 13:09:04 -0700 Subject: [PATCH] fix(app-shell): stop double-firing action error/success toasts (#2252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RecordDetailView's serverActionHandler (script actions) and useObjectActions' delete handler each toasted an error/success themselves AND returned it to the ActionRunner, whose post-execution hook toasts again when onToast is wired — so the same message fired twice (the reported RECORD_LOCKED double-toast, plus a double on delete success/failure). - RecordDetailView: drop the self toast.error in both failure branches; return the error and let the runner surface it once (mirrors #2177's twin fix in useConsoleActionRuntime). - useObjectActions.delete: keep the richer localized toast, return without `error` on failure and mark successful deletes `silent` so the runner doesn't re-toast. - Add useObjectActions.test.tsx asserting a single toast on delete success/failure/partial-bulk-failure. --- .../dedupe-recorddetail-delete-toast.md | 26 ++++ .../hooks/__tests__/useObjectActions.test.tsx | 141 ++++++++++++++++++ .../app-shell/src/hooks/useObjectActions.ts | 20 ++- .../app-shell/src/views/RecordDetailView.tsx | 11 +- 4 files changed, 190 insertions(+), 8 deletions(-) create mode 100644 .changeset/dedupe-recorddetail-delete-toast.md create mode 100644 packages/app-shell/src/hooks/__tests__/useObjectActions.test.tsx diff --git a/.changeset/dedupe-recorddetail-delete-toast.md b/.changeset/dedupe-recorddetail-delete-toast.md new file mode 100644 index 000000000..08e2346cc --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/hooks/__tests__/useObjectActions.test.tsx b/packages/app-shell/src/hooks/__tests__/useObjectActions.test.tsx new file mode 100644 index 000000000..9d63be063 --- /dev/null +++ b/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) => (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 }); + }); +}); diff --git a/packages/app-shell/src/hooks/useObjectActions.ts b/packages/app-shell/src/hooks/useObjectActions.ts index c554aa274..599a0309f 100644 --- a/packages/app-shell/src/hooks/useObjectActions.ts +++ b/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 }; } }); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 9a4ac16ed..7697ab825 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/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);